[GitHub] geode pull request #444: Feature/gem 1353
GitHub user gesterzhou opened a pull request: https://github.com/apache/geode/pull/444 Feature/gem 1353 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEM-1353 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/444.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #444 commit b9e8ac59892aed7f2ddea62f54b07ddac688456e Author: zhouxh Date: 2017-04-08T01:43:43Z GEM-1353 commit 72bc82e02f4ffb475903fc2b528f0f3dc37c1f9a Author: zhouxh Date: 2017-04-09T06:02:32Z fix-1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #444: Feature/gem 1353
Github user bschuchardt commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110697384
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
---
@@ -240,12 +240,37 @@ public boolean containsRegionContentChange() {
return true;
}
+ public long startOperation() {
+DistributedRegion region = getRegion();
+long viewVersion = -1;
+if (this.containsRegionContentChange()) {
+ viewVersion = region.getDistributionAdvisor().startOperation();
+}
+if (logger.isTraceEnabled(LogMarker.STATE_FLUSH_OP)) {
+ logger.trace(LogMarker.STATE_FLUSH_OP, "dispatching operation in
view version {}",
+ viewVersion);
+}
+return viewVersion;
+ }
+
+ public void endOperation(long viewVersion) {
+DistributedRegion region = getRegion();
+if (viewVersion != -1) {
+ region.getDistributionAdvisor().endOperation(viewVersion);
+ if (logger.isDebugEnabled()) {
+logger.trace(LogMarker.STATE_FLUSH_OP, "done dispatching operation
in view version {}",
+viewVersion);
+ }
+}
+ }
+
/**
* Distribute a cache operation to other members of the distributed
system. This method determines
* who the recipients are and handles careful delivery of the operation
to those members.
*/
public void distribute() {
--- End diff --
Decentralizing the start/end of the op seems fragile to me. Could you at
least change the javadocs to state that startOperation must be invoked before
distribute() and have the distribute() method throw an exception if
startOperation has not been invoked? Or maybe it would be even better to make
distribute() private and have startOperation invoke it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #444: Feature/gem 1353
Github user bschuchardt commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110699099
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
---
@@ -240,12 +240,37 @@ public boolean containsRegionContentChange() {
return true;
}
+ public long startOperation() {
--- End diff --
This and endOperation are missing javadocs, which I think are essential in
this case.
I don't think the callers of startOperation care that the return value is a
"viewVersion". I think you should change all of the methods invoking
startOperation to use "operationToken" instead of "viewVersion" so as not to
make that code confusing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #444: Feature/gem 1353
Github user bschuchardt commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110708948
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java ---
@@ -1261,7 +1333,14 @@ void basicUpdateEntryVersion(EntryEventImpl event)
throws EntryNotFoundException
}
protected void distributeUpdateEntryVersionOperation(EntryEventImpl
event) {
-new UpdateEntryVersionOperation(event).distribute();
+UpdateEntryVersionOperation op = new
UpdateEntryVersionOperation(event);
+long viewVersion = -1;
+try {
+ viewVersion = op.startOperation();
--- End diff --
For cases like this one it would be nice to move the try/finally to a new
method in DistributedCacheOperation and just invoke that method. Maybe call it
distribute()???
```
public void distributed() {
long token;
try {
token = startOperation();
_distribute(); // private method
} finally {
endOperation(token);
}
}
```
If you do this you can revert the changes you've made to a lot of classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #444: Feature/gem 1353
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110719505
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
---
@@ -240,12 +240,37 @@ public boolean containsRegionContentChange() {
return true;
}
+ public long startOperation() {
+DistributedRegion region = getRegion();
+long viewVersion = -1;
+if (this.containsRegionContentChange()) {
+ viewVersion = region.getDistributionAdvisor().startOperation();
+}
+if (logger.isTraceEnabled(LogMarker.STATE_FLUSH_OP)) {
+ logger.trace(LogMarker.STATE_FLUSH_OP, "dispatching operation in
view version {}",
+ viewVersion);
+}
+return viewVersion;
+ }
+
+ public void endOperation(long viewVersion) {
+DistributedRegion region = getRegion();
+if (viewVersion != -1) {
--- End diff --
If somehow we get a -1, will the state flush ever "end?" or are we stuck?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #444: Feature/gem 1353
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110702026
--- Diff:
geode-cq/src/test/java/org/apache/geode/cache/query/cq/dunit/CqQueryDUnitTest.java
---
@@ -1577,7 +1577,13 @@ public void run2() throws CacheException {
Region subregion = getCache().getRegion("root/" + regionName);
DistributedTombstoneOperation gc = DistributedTombstoneOperation
.gc((DistributedRegion) subregion, new
EventID(getCache().getDistributedSystem()));
-gc.distribute();
+long viewVersion = -1;
+try {
+ viewVersion = gc.startOperation();
+ gc.distribute();
+} finally {
+ gc.endOperation(viewVersion);
--- End diff --
Same as my other question earlier, if an error occurs during start
operation, viewVersion could potentially be -1 still. Does this handle that
behavior correctly? We do this try/finally pattern in a lot of places, as
Bruce stated, maybe we could combine into a common method (there are some cases
where this may not be possible)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #444: Feature/gem 1353
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110718931
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java ---
@@ -1261,7 +1333,14 @@ void basicUpdateEntryVersion(EntryEventImpl event)
throws EntryNotFoundException
}
protected void distributeUpdateEntryVersionOperation(EntryEventImpl
event) {
-new UpdateEntryVersionOperation(event).distribute();
+UpdateEntryVersionOperation op = new
UpdateEntryVersionOperation(event);
+long viewVersion = -1;
+try {
+ viewVersion = op.startOperation();
--- End diff --
Out of curiosity, what happens if the startOperation() method returns a -1
(I think that may be possible based on how it's currently written or if an
exception is thrown before token is assigned. What is the expected way to
handle endOperation with an invalid/-1 token?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #444: Feature/gem 1353
Github user asfgit closed the pull request at: https://github.com/apache/geode/pull/444 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
