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:
[email protected]
