----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50715/#review146054 -----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 37) <https://reviews.apache.org/r/50715/#comment212418> make sure and remove the commented out RegionListener geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 116) <https://reviews.apache.org/r/50715/#comment212419> Collections.emptyList() is better geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 118) <https://reviews.apache.org/r/50715/#comment212420> couldn't you change lines 113..122 to be: missingChildren.removeAll(coloHierarchy.keySet()); geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 129) <https://reviews.apache.org/r/50715/#comment212421> I don't think you want to log an error because the thread was interrupted. In this thread's case I think you should just return if interrupted. I think the only thing you need to do in this particular catch is: Thread.currentThread().interrupt(); that will reset the interrupt bit that was cleared when you caught the exception. You really don't even need that but it is always a good idea in case something else after the return ends up blocking and should have also been interrupted. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 151) <https://reviews.apache.org/r/50715/#comment212423> I'd like to see you reuse "addMissingChildRegion" here instead of duplicating that code. So this method could be: for (String name: childRegion.getMissingColocatedChildren()) { addMissingChildRegion(name); } geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java (line 7490) <https://reviews.apache.org/r/50715/#comment212424> I think you need to deal with missingColoctedRegionLogger being null. And it can become null async if stop is called. So this method should read it into a local var; test the local var; if not null call getMissingChildRegions; otherwise return Collections.emptyList(). - Darrel Schneider On Aug. 17, 2016, 4:12 p.m., Ken Howe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50715/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2016, 4:12 p.m.) > > > Review request for geode, Darrel Schneider, Eric Shu, Scott Jewell, Kirk > Lund, and Dan Smith. > > > Repository: geode > > > Description > ------- > > Test for missing parent regions on child region creates and throw > IllegalStateException > > Log warnings about missing colocated child regions. > > Create Unit and DUnit tests for new exceptions and warnings > > There are two cases of missing colocated regions, > 1) The 'parent' region hasn't been created when a region specifies it in the > 'colocated-with' attribute > 2) A persistent child region does not exist. > > For (1), this condition can be determined in > ColocationHelper.getColocatedRegion(). The core product currently does not > test for this which results in an NPE being thrown without any logging to > indicate the reason. There are two variations of this state. > > 1a) When starting a region with non-null 'colocated'with', a reference to the > parent region configuration is obtained through the configuration root. When > the reference obtained is null (the region doesn't exist in the root > configuration) the NPE ends up getting thrown. The fix for this is to > immediately throw an IllegalStateException with a message to note the missing > colocated-with region. > > 1b) The parent region configuration may exist in the root configuration (the > parent PR has been created on another member) but does not exist on the local > member. In this case the null comes about when obtaining the local region > (PartitionedRegion.getPRFromId()). The fix here is the same as (1a) - throw > an IllegalStateException. > > For (2), missing child regions: This state will always exist for an > indeterminate period because the parent is always created before the child > region. There currently isn't any indication in the logs of this condition, > even it if persists indefinitely, other than a failure to recover the PR's > persistent data. The fix for this is starting a logging thread (similar to > the RedundancyLogger) when a child region is found missing. The condition > will be periodically logged (set initially to 30 secs) until the region is > created. There is a delay before the first log message to allow time for the > normal sequencing of region creations. > > > Diffs > ----- > > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationHelper.java > 012a77f > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java > PRE-CREATION > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionRegionConfig.java > 6d7c1ca > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java > 9ac95a1 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/RedundancyLogger.java > f7e8621 > > geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java > 443fe78 > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ColocationHelperTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java > d8b3514 > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentPartitionedRegionTestBase.java > 692378c > > Diff: https://reviews.apache.org/r/50715/diff/ > > > Testing > ------- > > precheckin in progress > > > Thanks, > > Ken Howe > >
