You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Eric Shu <es...@pivotal.io> on 2017/05/19 15:38:00 UTC
Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
Bugs: GEODE-2939
https://issues.apache.org/jira/browse/GEODE-2939
Repository: geode
Description
-------
Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
Diff: https://reviews.apache.org/r/59404/diff/1/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175729
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Line 250 (original), 252 (patched)
<https://reviews.apache.org/r/59404/#comment249084>
I think you should get rid of the eventStateLock.
It seems wrong for you to get the map and do a put instead of just calling a single method.
I also don't like the "instanceof BucketRegion."Just call a new method on "lr" named recordPendingEventState that takes the sender and the eventState.
On LocalRegion this method can do nothing.
Override it on BucketRegion to synchronize on the remoteEventStates map.
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 543 (patched)
<https://reviews.apache.org/r/59404/#comment249086>
Once again, get rid of 'instanceof BucketRegion' by having a noop method on whatever "this.region" is and then override that on BucketRegion.
Also all the code in this if block should be in the method on BucketRegion.
You might also want to do something better than remoteEventStates.clear() because if a createRegionReply comes in after this you will store its eventState in this BucketRegion and waste memory until this bucket is finally destroyed.
Would it be possible to register the processor with the region and have the processor store all the pending event states? Then when you finally finish by calling recordEventState you could just null out the registered processor.
- Darrel Schneider
On May 19, 2017, 8:38 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 8:38 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/1/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175825
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
Lines 288 (patched)
<https://reviews.apache.org/r/59404/#comment249163>
To avoid a race condition that results in an NPE do this:
rp = this.createRegionReplyProcessor;
if (rp != null) {
rp.getRemoteEventStates().put(member, eventStates);
}
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
Lines 301 (patched)
<https://reviews.apache.org/r/59404/#comment249166>
Instead of exposing the entire private Map by having a getRemoteEventStates method, just add a getEventState(InternalDistributedMember) method and use that here.
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Lines 100 (patched)
<https://reviews.apache.org/r/59404/#comment249159>
instead of casting to LocalRegion it would be better to add "registerCreateRegionReplyProcessor" to CacheDistributionAdvisee. Add it as a "default" method that does nothing. Then you only need to override it on BucketRegion. No need to add a noop to LocalRegion.
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Line 202 (original), 204 (patched)
<https://reviews.apache.org/r/59404/#comment249158>
make this final
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Line 250 (original), 252 (patched)
<https://reviews.apache.org/r/59404/#comment249160>
I see no need for you to go to "lr" for this. Can't you record it locally in your own 'remoteEventStates' map? This allows you to completely get rid of the recordPendingEventState.
Also get rid of "lr.hasEventTracker". If you get a non-null eventState back just stick it in your local remoteEventStates map.
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 542 (patched)
<https://reviews.apache.org/r/59404/#comment249161>
This comment is not needed. You method name makes this clear.
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 543 (patched)
<https://reviews.apache.org/r/59404/#comment249162>
Move this call down to line 554 right before the call of endGetInitialImage
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 544 (patched)
<https://reviews.apache.org/r/59404/#comment249165>
Since "this.region" is a DistributedRegion it would be best for this method to start on it instead of LocalRegion.
- Darrel Schneider
On May 23, 2017, 10:55 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 10:55 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 8e7ec68
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/2/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Eric Shu <es...@pivotal.io>.
> On May 24, 2017, 4:59 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
> > Lines 209 (patched)
> > <https://reviews.apache.org/r/59404/diff/3/?file=1730477#file1730477line215>
> >
> > Why is this Map <?,?> ?
> > I can see on EventTracker.recordState we have: Map<ThreadIdentifier, EventSeqnoHolder>
> >
> > But then on LocalRegion.recordEventState we just have "Map". Would it be hard to fix this to consistently use Map<ThreadIdentifier, EventSeqnoHolder>?
Will not be able to change the LocalRegion impl as HARegionQueue overrides the LocalRegion.recordEventState, and it does not uses Map<ThreadIdentifier, EventSeqnoHolder>.
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175956
-----------------------------------------------------------
On May 25, 2017, 12:51 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 12:51 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java e4a7957
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java 485835b
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/4/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175956
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
Lines 293 (patched)
<https://reviews.apache.org/r/59404/#comment249293>
Can this method ever be invoked concurrently by two different threads?
geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java
Lines 60 (patched)
<https://reviews.apache.org/r/59404/#comment249303>
Change this comment to not talk about BucketRegion.
You can do that down on BucketRegion override of this method if needed.
Here I would just say something like:
Allows this advisee to know the CreateRegionReplyProcessor that is creating it.
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Lines 209 (patched)
<https://reviews.apache.org/r/59404/#comment249305>
Why is this Map <?,?> ?
I can see on EventTracker.recordState we have: Map<ThreadIdentifier, EventSeqnoHolder>
But then on LocalRegion.recordEventState we just have "Map". Would it be hard to fix this to consistently use Map<ThreadIdentifier, EventSeqnoHolder>?
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Line 259 (original), 255 (patched)
<https://reviews.apache.org/r/59404/#comment249307>
I'm not sure why we bother nulling out reply.eventState but at the very least this line should be moved into the if (rely.eventState != null)
- Darrel Schneider
On May 23, 2017, 3:32 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 3:32 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java e4a7957
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java 485835b
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/3/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175971
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
Lines 293 (patched)
<https://reviews.apache.org/r/59404/#comment249324>
Only one thread will invoke this method from InitialImageOperation.getFromOne.
- Eric Shu
On May 25, 2017, 12:51 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 12:51 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java e4a7957
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java 485835b
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/4/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review176146
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Line 203 (original), 207 (patched)
<https://reviews.apache.org/r/59404/#comment249484>
You need this map to be thread safe because concurrent puts can happen as you process multiple replies concurrently.
You can wrap it in either Collections.synchronizedMap or you can make it a ConcurrentHashMap.
Also you could make this line less verbose by just using "<>" on the new side (I think).
- Darrel Schneider
On May 24, 2017, 5:51 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 5:51 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java e4a7957
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java 485835b
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/4/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/
-----------------------------------------------------------
(Updated May 25, 2017, 12:51 a.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
Changes
-------
fix review comments.
Bugs: GEODE-2939
https://issues.apache.org/jira/browse/GEODE-2939
Repository: geode
Description
-------
Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java e4a7957
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java 485835b
geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
Diff: https://reviews.apache.org/r/59404/diff/4/
Changes: https://reviews.apache.org/r/59404/diff/3-4/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/
-----------------------------------------------------------
(Updated May 23, 2017, 10:32 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
Changes
-------
fix review comments
Bugs: GEODE-2939
https://issues.apache.org/jira/browse/GEODE-2939
Repository: geode
Description
-------
Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
geode-core/src/main/java/org/apache/geode/internal/cache/CacheDistributionAdvisee.java e4a7957
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java 485835b
geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
Diff: https://reviews.apache.org/r/59404/diff/3/
Changes: https://reviews.apache.org/r/59404/diff/2-3/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/
-----------------------------------------------------------
(Updated May 23, 2017, 5:55 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
Changes
-------
fix review comments
Bugs: GEODE-2939
https://issues.apache.org/jira/browse/GEODE-2939
Repository: geode
Description
-------
Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 8e7ec68
geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
Diff: https://reviews.apache.org/r/59404/diff/2/
Changes: https://reviews.apache.org/r/59404/diff/1-2/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by Eric Shu <es...@pivotal.io>.
> On May 23, 2017, 12:34 a.m., anilkumar gingade wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
> > Line 251 (original), 253 (patched)
> > <https://reviews.apache.org/r/59404/diff/1/?file=1725072#file1725072line256>
> >
> > Do we need to assert here or do as in following logic (line#263):
> > if (lr.isUsedForPartitionedRegionBucket())
> >
> > It looks like, this could be called for non-pr bucket region...Where is the recordEvent set for non bucket region.
Only BucketRegion sends event states back in CreateRegionReplyMessage, this assert is to alert future changes if any. But it is no longer needed as it is handled in the BucketRegion now.
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175732
-----------------------------------------------------------
On May 19, 2017, 3:38 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 3:38 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/1/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 59404: GEODE-2939: make sure event tracker is
initiated from the GII provider
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59404/#review175732
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java
Line 251 (original), 253 (patched)
<https://reviews.apache.org/r/59404/#comment249087>
Do we need to assert here or do as in following logic (line#263):
if (lr.isUsedForPartitionedRegionBucket())
It looks like, this could be called for non-pr bucket region...Where is the recordEvent set for non bucket region.
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 543 (patched)
<https://reviews.apache.org/r/59404/#comment249088>
instead of instance check we could use region.isUsedForPartitionedRegionBucket()
Is this needs to be in for loop...Or can this be moved after for loop under if (this.gotImage())
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 546 (patched)
<https://reviews.apache.org/r/59404/#comment249091>
Sorry for nit picking...
will it be more readble to switch the condition...
if (providerEfventStates != null) {
set
} else {
log
}
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
Lines 556 (patched)
<https://reviews.apache.org/r/59404/#comment249090>
It will be cleaner to have this in BucketRegion where the map is created.
getRemoteEventStates()
clearRemoteEventStates()
- anilkumar gingade
On May 19, 2017, 3:38 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59404/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 3:38 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn Gallinat.
>
>
> Bugs: GEODE-2939
> https://issues.apache.org/jira/browse/GEODE-2939
>
>
> Repository: geode
>
>
> Description
> -------
>
> Event tracker initilization for bucket region is delayed until after GII, and will be initialized from the GII provider.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 886d678
> geode-core/src/main/java/org/apache/geode/internal/cache/CreateRegionProcessor.java c1d1e77
> geode-core/src/main/java/org/apache/geode/internal/cache/EventTracker.java 2c86aed
> geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java fb5f0cf
> geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java 3faf41f
>
>
> Diff: https://reviews.apache.org/r/59404/diff/1/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>