I typically use 2 MINUTES as the timeout for everything I've worked on in integration or distributed tests. I believe that whatever the underlying async action is will still complete well under 2 MINUTES even if a long GC pause occurs during that window of time. 5 MINUTES is probably longer than we need but at least it'll be ok on very slow hardware.
On Thu, Jul 12, 2018 at 10:21 AM, Kirk Lund <kl...@apache.org> wrote: > I recommend keeping this Awaitility with default timeout separate from any > Rules or MemberVM. I want to be able to use Awaitility in integration tests > and distributed tests that don't use MemberVM or the StarterRules. So from > that POV, I'd prefer the GeodeAwaitility that Dan proposes. > > On Thu, Jul 12, 2018 at 9:48 AM, Patrick Rhomberg <prhomb...@pivotal.io> > wrote: > >> +1 to a single source of member wait calls. >> >> We already have a standard set of await methods for DUnit member VMs, >> located in the MemberVM class, and delegated to that class from the >> MemberStarterRule. I have a PR outstanding [1] that improves those >> methods, too. >> >> For those awaits that are common for VM members, that may be a more >> natural >> place for these methods to live. >> >> Imagination is Change. >> ~Patrick >> >> [1] PR 2039, https://github.com/apache/geode/pull/2039, currently just >> waiting for a precheckin after a merge with develop. >> >> On Wed, Jul 11, 2018 at 1:03 PM, Mark Hanson <mhan...@pivotal.io> wrote: >> >> > After further discussion, based on GC variability +1 for what Dan said. >> > >> > Thanks, >> > Mark >> > >> > > On Jul 11, 2018, at 12:53 PM, Jacob Barrett <jbarr...@pivotal.io> >> wrote: >> > > >> > > +1 what Dan said. >> > > >> > >> On Jul 11, 2018, at 11:16 AM, Dan Smith <dsm...@pivotal.io> wrote: >> > >> >> > >> Well, some of these tests are waiting for members to startup, etc. If >> > the >> > >> machine they are running on is slow, that could take more than a >> minute. >> > >> >> > >> The point here is that these are not tests of how long it takes do a >> > geode >> > >> operation. That's what performance tests are for. These tests just >> have >> > an >> > >> atMost because we want them to fail, rather than hang, if the >> assertion >> > is >> > >> never satisfied. Because these tests should always pass in a variety >> of >> > >> environments, we should set atMost to be something really large. >> > >> >> > >> Performance tests that have assertions about timing need to run on >> known >> > >> hardware, and generally need to assert things like 99.9% the response >> > time >> > >> is within this window. That's not what this test suite is. >> > >> >> > >> -Dan >> > >> >> > >>> On Wed, Jul 11, 2018 at 10:05 AM, Udo Kohlmeyer <u...@apache.org> >> > wrote: >> > >>> >> > >>> Hi there Dan, >> > >>> >> > >>> Whilst 5min seems to be a viable option, I believe that knowing how >> > long >> > >>> an operation should take and reacting if it doesn't complete in that >> > time >> > >>> is better than waiting a standard amount of time. I like the faster >> > >>> feedback option, rather than the standard timeout across the board. >> > Now, >> > >>> that said, crazy timeouts like 1-10s are maybe a little low. >> > >>> >> > >>> Maybe 1min rather than 5min? With exception to disk recovery tests, >> > which >> > >>> feasibly could take more than 1min. I cannot think of a single >> > operation in >> > >>> Geode, that should realistically should take more than 60s. >> > >>> >> > >>> --Udo >> > >>> >> > >>> >> > >>> >> > >>>> On 7/11/18 09:07, Dan Smith wrote: >> > >>>> >> > >>>> Hi all, >> > >>>> >> > >>>> We have a bunch of tests that are using awaitility. It seems like >> > every >> > >>>> tests is picking some random number of it's timeout, usually in the >> > range >> > >>>> of 10-60 seconds. >> > >>>> >> > >>>> I'd like to change all of our tests to use a standard timeout that >> is >> > much >> > >>>> higher, to avoid worrying about whether our timeouts are to low. >> So I >> > >>>> propose introducing our own GeodeAwaitility class that sets a >> timeout >> > of 5 >> > >>>> minutes and replacing all of our usage with that. >> > >>>> >> > >>>> -Dan >> > >>>> >> > >>>> >> > >>> >> > >> > >> > >
