[GitHub] [curator] cammckenzie commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations

[GitBox <gi...@apache.org>]

cammckenzie commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r564812866



##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -87,6 +93,7 @@ public CreateBuilderImpl(CuratorFrameworkImpl client, CreateMode createMode, Bac
         this.createParentsAsContainers = createParentsAsContainers;
         this.compress = compress;
         this.setDataIfExists = setDataIfExists;
+        this.idempotent = false; // TODO should I add this to async framework as well? Looks like that's the reason this is a public constructor

Review comment:
       For now I think just leave this as defaulting to false, and we can add to the async framework in another PR.

##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java
##########
@@ -173,6 +186,11 @@ public void performBackgroundOperation(final OperationAndData<String> operationA
                         @Override
                         public void processResult(int rc, String path, Object ctx)
                         {
+                            if ( (rc == KeeperException.Code.OK.intValue()) && failNextDeleteForTesting )
+                            {
+                                failNextDeleteForTesting = false;
+                                rc = KeeperException.Code.CONNECTIONLOSS.intValue();
+                            }
                             trace.setReturnCode(rc).setPath(path).commit();

Review comment:
       Just a nit pick as this won't happen outside testing, but I think that the trace.setReturnCode should be called at the top of the method.

##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -228,10 +289,25 @@ public void performBackgroundOperation(final OperationAndData<PathAndBytes> oper
                     @Override
                     public void processResult(int rc, String path, Object ctx, Stat stat)
                     {
+                        if ( (rc == KeeperException.Code.OK.intValue()) && failNextSetForTesting )
+                        {
+                            failNextSetForTesting = false;
+                            rc = KeeperException.Code.CONNECTIONLOSS.intValue();
+                        }
+
                         trace.setReturnCode(rc).setRequestBytesLength(data).setPath(path).setStat(stat).commit();

Review comment:
       Move trace to top of method

##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -51,6 +62,8 @@ public SetDataBuilderImpl(CuratorFrameworkImpl client, Backgrounding backgroundi
         this.backgrounding = backgrounding;
         this.version = version;
         this.compress = compress;
+        // TODO jslocum - this is only public for x-async api. Should I add idempotent there too?

Review comment:
       I think just leave it for now and default idempotent to false. Can update with a subsequent PR if required.

##########
File path: pom.xml
##########
@@ -68,7 +68,7 @@
 
         <!-- versions -->
         <zookeeper-version>3.6.0</zookeeper-version>
-        <maven-bundle-plugin-version>4.1.0</maven-bundle-plugin-version>
+        <maven-bundle-plugin-version>5.1.1</maven-bundle-plugin-version>

Review comment:
       I assume this version upgrade won't have any unexpected impacts, but my familiarity with Maven is pretty minimal. @eolivelli @Randgalt thoughts?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org