----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58550/#review172513 -----------------------------------------------------------
I think the main thrust of this change looks good - get the AEQ created early enough to avoid losing data. This seems like the right way to do that! I have some comments about cleaning up the code below. geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/AbstractPartitionedRepositoryManager.java Lines 57 (patched) <https://reviews.apache.org/r/58550/#comment245612> Should regionPath really be null for a period of time? geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneEventListener.java Lines 58 (patched) <https://reviews.apache.org/r/58550/#comment245616> Ready should be volatile if it is used by a separate thread - but in this case do we really need a separate ready flag? CountDownLatch.await will just return once countDown has been called. geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneEventListener.java Lines 71 (patched) <https://reviews.apache.org/r/58550/#comment245628> Might need to wait in a loop and check a CancelCriterion or something. It's possible the cache gets closed before the latch is counted down, maybe? geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneEventListener.java Lines 73 (patched) <https://reviews.apache.org/r/58550/#comment245613> Is this really how we want to handle interrupts here? If this thread is interrupted, just move on? Seems like you might want to try to wait uninterruptibly. geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexForPartitionedRegion.java Lines 63 (patched) <https://reviews.apache.org/r/58550/#comment245626> Maybe rename this method, since it also sets the region path on the index? geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexForPartitionedRegion.java Line 59 (original), 69 (patched) <https://reviews.apache.org/r/58550/#comment245621> Duplicate code - blow away the old createRepositoryManager if we are not using it. geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexForPartitionedRegion.java Lines 114 (patched) <https://reviews.apache.org/r/58550/#comment245619> Maybe come up with a better name than "createJustRepoManager?" geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexImpl.java Line 152 (original), 207 (patched) <https://reviews.apache.org/r/58550/#comment245622> This looks like more duplicate code. Remove the old code. geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexImpl.java Lines 249 (patched) <https://reviews.apache.org/r/58550/#comment245627> Commented out code. geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneRegionListener.java Lines 119 (patched) <https://reviews.apache.org/r/58550/#comment245623> Why would the aeq be null here? geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneServiceImpl.java Line 170 (original), 171 (patched) <https://reviews.apache.org/r/58550/#comment245624> Remove commented out code. - Dan Smith On April 20, 2017, 2:03 a.m., nabarun nag wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58550/ > ----------------------------------------------------------- > > (Updated April 20, 2017, 2:03 a.m.) > > > Review request for geode, Jason Huynh and Dan Smith. > > > Repository: geode > > > Description > ------- > > Testing a new start up mechanism where the AEQ is created before the user > region. Please review and let us know if any modifications are needed, or if > this is a viable solution > > > Diffs > ----- > > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/AbstractPartitionedRepositoryManager.java > 26bb488ed > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneEventListener.java > 0f5553343 > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexForPartitionedRegion.java > fea484547 > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexImpl.java > 36f6720c3 > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneIndexImplFactory.java > e99f3d9db > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneRawIndex.java > 75ab5cab3 > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneRegionListener.java > f4e2a79ef > > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneServiceImpl.java > 30952bfe2 > > geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/LuceneEventListenerJUnitTest.java > 79de29a09 > > geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/LuceneIndexForPartitionedRegionTest.java > 8e4c179a5 > > > Diff: https://reviews.apache.org/r/58550/diff/1/ > > > Testing > ------- > > > Thanks, > > nabarun nag > >
