> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationHelper.java,
> > line 226
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460784#file1460784line226>
> >
> > 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).
Using one thread that monitors all missing children made for reformatting hte
warning messages. Logs now reported only children that are missing at the time
hte log is created. If/when child regions are created the message then the
logger removes them from the missing children list. Typical log showing two
missing child regions:
Persistent data recovery is prevented by offline colocated regions /region3,
/region2
Region initialization is incomplete for:
/PersistentColocatedPartitionedRegionDUnitTest_testMultipleColocatedChildPRsMissingWithSequencedStartRegion
In the case of a colocation hierarchy all affected regions back to the root of
the hierarchy are listed in the log.
For example, where /region1 is the root of the hierarchy and
/region3 specifies 'colocated-wtih' /region2
/region2 specifies 'colocated-wtih' /region1
The log shows:
Persistent data recovery is prevented by offline colocated region /region3
Region initialization is incomplete for:
/region2
/region1
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java,
> > line 40
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460785#file1460785line40>
> >
> > A better name would be "childFullPath"
With the change to logger-thread per region, this variable was removed.
Instead, the ColocationLogger has a list named missingChilren
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java,
> > line 41
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460785#file1460785line41>
> >
> > add final on the inst vars that never change
Named change to loggerLock
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java,
> > line 49
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460785#file1460785line49>
> >
> > 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?)
milliseconds noted in the javadoc comment.
LOG_INTERVAL is static to supprot the test hooks. Hooks are used to keep test
runtimes reasonable
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java,
> > line 78
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460785#file1460785line78>
> >
> > 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.
synchronization was refactored along with the change to logger-thread per region
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java,
> > line 306
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460787#file1460787line306>
> >
> > can this be made final?
This is no longer a List, it's now variable to hold the ColocationLogger
reference for the region. It changes during the life of the region so it can't
be final.
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java,
> > line 7464
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460787#file1460787line7464>
> >
> > 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);
> > }
Code was refactored with the change to logger-thread per region
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java,
> > line 7468
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460787#file1460787line7468>
> >
> > This sync and notify should all be in the setRegionDestroyed method.
> > getLoggerLock should be private
Synchronization with the logger thread was refactored.
> On Aug. 2, 2016, 11:54 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java,
> > line 3764
> > <https://reviews.apache.org/r/50715/diff/1/?file=1460788#file1460788line3764>
> >
> > 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.
Message was reformatted. See comments re: changing to the logger-thread per
region.
- Ken
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50715/#review144540
-----------------------------------------------------------
On Aug. 18, 2016, 6:17 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50715/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2016, 6:17 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/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
>
>
