+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 > >>>> > >>>> > >>> > >
