----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50715/#review145536 -----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationHelper.java (line 231) <https://reviews.apache.org/r/50715/#comment211820> This is the grandchild case right? I don't see it adding anything to the ColocationLogger so how does that work? geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 80) <https://reviews.apache.org/r/50715/#comment211803> I don't see a need to for this for loop. It actually does more work than needed because once it finds a match it should have just returned. Logically I think you just wanted to do this: String fullPath = region.getFullPath(); if (missingChildren.contains(fullPath) { removeMissingChildRegion(fullPath); } But the other thing you are doing is a double check. You test if it is in the collection here and then if it is you call removeMissingChildRegion; it syncs; and then you basically do this check again. This does avoid the sync on a miss but do you really need this? Couldn't this method just call: removeMissingChildRegion(region.getFullPath()); geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 99) <https://reviews.apache.org/r/50715/#comment211821> This was the old code style for catch ( "} LF catch"). I think you got this code from and old class. In this new class I think you should convert this to the new style "} catch". geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 126) <https://reviews.apache.org/r/50715/#comment211815> !isEmpty() better than size() > 0 same goes for line 139 geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 129) <https://reviews.apache.org/r/50715/#comment211817> This name (warningLogged) is a bit misleading since we might not actually log a warning even when it is set to true. I change to something like "alreadySlept". Setting it to true should happen inside the if that tests for it to be false instead of everytime around the loop. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 130) <https://reviews.apache.org/r/50715/#comment211818> sleepMillis /= 2; is a bit clearer geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 134) <https://reviews.apache.org/r/50715/#comment211812> This does not seem like the correct handling of InterruptedException. I think in this thread if wait is interrupted you just want the thread to terminate. The way you currently have it coded up it can't be interrupted. I would move this catch to your outer try and get rid of this inner try completely. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 135) <https://reviews.apache.org/r/50715/#comment211808> Why is catch Exception needed? Seems like anything it would catch should just be thrown and your code up in run will handle it. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 169) <https://reviews.apache.org/r/50715/#comment211792> Also is it seems like you might as well check the result of remove and only do the isEmpty check if remove returned non-null. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 170) <https://reviews.apache.org/r/50715/#comment211787> I noticed that here you have "this.missingChildren" but in other places you drop the "this.". We used to require that instance var references included "this.". It was part of our coding guidelines. But given IDEs I'd vote for not adding "this." unless it is required because of a local var having the same name as an inst var. Since you have a mix in this class I'd recommend that you can this new class for "this." that you can remove and do so. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 171) <https://reviews.apache.org/r/50715/#comment211788> Your "getMissingChildRegions" clones the "missingChildren" collection. No need for that here since you are synced on the loggerLock. Also it is better to call isEmpty() instead of size() == 0. So I'd change this line to: if (missingChildren.isEmpty()) geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 178) <https://reviews.apache.org/r/50715/#comment211795> I see no reason for you doing this clone. The whole point of CopyOnWriteArrayList is to provide safe concurrent access. It looks like this method is for afterCreate and I don't see a need for it to get a clone. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 200) <https://reviews.apache.org/r/50715/#comment211825> I see. This is a bit different than what I expected. But it might be ok. I thought an offline region would list the regions that are causing it to be offline. If a direct child does not exist then it does log that. But if a direct child is offline because a grandchild is missing then this offline region will not list that it is offline bcause of the grandchild. With your way of doing it the offline child will list that it is offline because of its missing child (its parent's grandchild) and it will list that it is keeping its parent offline. This seems a bit indirect. It is possible you will not have any log message that says the parent is offline because... Instead you will have a log message about its child that also says it is causing a problem for its parent. This is better than before. Lets talk about this one. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java (line 206) <https://reviews.apache.org/r/50715/#comment211822> bad indentation on this line - Darrel Schneider On Aug. 11, 2016, 10:51 a.m., Ken Howe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50715/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2016, 10:51 a.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/i18n/LocalizedStrings.java > 2254a89 > > 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 > >
