That PR was reverting the wrong commit, so I've closed it. I'll submit a new PR today to revert the introduction of WanCopyRegionFunction and its unit test. I'm going to work with Alberto next week to address some testability issues so it can be reintroduced as soon as possible.
Thanks, Kirk On Thu, Aug 26, 2021 at 4:56 PM Kirk Lund <kl...@apache.org> wrote: > I have submitted PR #6809 to temporarily revert GEODE-9408 until it's > ready to go back in. > > https://github.com/apache/geode/pull/6809 > > I've reviewed the code that went in with #b377e3, and I think we should > revert it to rework WanCopyRegionFunction and its unit test. The unit test > currently mocks three methods in WanCopyRegionFunction (known as partial > mocking) which is a sign that dependencies are hidden inside the class and > need to be pulled up to the constructor. > > WanCopyRegionFunction also has a static final ExecutorService which is > never shutdown. The ExecutorService should live outside > WanCopyRegionFunction, be passed in via the constructor, and be shutdown > when the Cache closes. > > The intermittent test failures are not specific to Windows and are likely > to also fail on a slow Linux machine. > > We really need to avoid Thread sleeps especially in unit tests. This and > the partial mocking are signs that the class needs to change to better > enable unit testing. > > I'll be more than happy to help but I think we need to revert it so we > have time to discuss it without everyone else being affected by failing > tests. > > Thanks, > Kirk >
