----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50715/#review144540 -----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationHelper.java (line 226) <https://reviews.apache.org/r/50715/#comment210593> Instead of a thread per missing child how about just one thread that monitors all the missing children? Instead of the PartitionedRegino having a list have ColocationLoggers it could have a single ColocationLogger and the ColocationLogger could have a list of missing children. Seems like this might scale better and make it easier for an offline reason the log all of the reasons it is still offline (it could log that it has children that exist but that are themselves offline). geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationHelper.java (line 401) <https://reviews.apache.org/r/50715/#comment210552> Seems like you should change: getRegionIdentifier(partitionedRegion.getFullPath()) to: partitionedRegion.getRegionIdentifier() geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 36) <https://reviews.apache.org/r/50715/#comment210553> any reason for keeping the commented out stuff? geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 40) <https://reviews.apache.org/r/50715/#comment210563> A better name would be "childFullPath" geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 41) <https://reviews.apache.org/r/50715/#comment210561> add final on the inst vars that never change geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 49) <https://reviews.apache.org/r/50715/#comment210555> make the time units (millis) clear. Also seems like LOG_INTERVAL should be and inst var instead of static (or does it need to be static for the test hooks?) geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 52) <https://reviews.apache.org/r/50715/#comment210554> typo geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 78) <https://reviews.apache.org/r/50715/#comment210567> I would put this sync on setMissingChildRegions. Also this seems like when you would also want to do a notify so the thread could wakeup and detect that it is no longer needed. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 118) <https://reviews.apache.org/r/50715/#comment210557> something is missing between "that of". geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 125) <https://reviews.apache.org/r/50715/#comment210556> get rid of "== true". I see it multiple times. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 141) <https://reviews.apache.org/r/50715/#comment210558> Get rid of "printStackTrace". The logger will log the stack trace. Once is enough. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 146) <https://reviews.apache.org/r/50715/#comment210559> This should not be a bitwise operator "&". Instead it should be logical op "&&". geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 151) <https://reviews.apache.org/r/50715/#comment210565> This notify does not make sense. You should not be notifing yourself. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java (line 306) <https://reviews.apache.org/r/50715/#comment210576> can this be made final? geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java (line 7464) <https://reviews.apache.org/r/50715/#comment210582> I think it is bad practice to have assignment it expressions. Also I don't see any need for this isEmpty check. I think this method body could just be: for (ColocationLogger offlineChildLogger: getOfflineColcatedLoggers()) { offlineChildLogger.setRegionDestroyed(true); } geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java (line 7468) <https://reviews.apache.org/r/50715/#comment210574> This sync and notify should all be in the setRegionDestroyed method. getLoggerLock should be private geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java (line 7475) <https://reviews.apache.org/r/50715/#comment210583> typo: "Colcated" shouldbe "Colocated" geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java (line 3763) <https://reviews.apache.org/r/50715/#comment210570> Instead of "is not present" how about "must be created" or "does not exist". geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java (line 3764) <https://reviews.apache.org/r/50715/#comment210571> Since this will show up as a warning I think we need it to give more detailed information. We need to tell them that the region they did create will not function because of the missing child. - Darrel Schneider On Aug. 2, 2016, 2:23 p.m., Ken Howe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50715/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2016, 2:23 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 > 012a77f3eeaaa5d1c782c6f868ced017a9a26163 > > 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 > 6d7c1ca6c1588bf5f36a021a5d6e9c9a97cd7487 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java > 9ac95a1f9c0ab880d6cab8a350bd9c4842797d5d > > geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java > 2254a895f2cfdb189727fe8cdce3f770c9c48364 > > 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 > d8b351417575c3e164e634a91f3ff0e7218860ed > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentPartitionedRegionTestBase.java > 692378c6dacf5ea4def4f1dbfec7744beb2bc9c6 > > Diff: https://reviews.apache.org/r/50715/diff/ > > > Testing > ------- > > precheckin in progress > > > Thanks, > > Ken Howe > >
