You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2021/02/09 20:56:04 UTC
[GitHub] [curator] jmslocum16 opened a new pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
jmslocum16 opened a new pull request #376:
URL: https://github.com/apache/curator/pull/376
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-781640693
Thanks for all your work getting this over the line @jmslocum16!
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-766454397
Thanks @jmslocum16, will try and take a look in the next few day.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-763709723
@cammckenzie Sorry for the delay on this - I was busy with work and the holidays.
I just completed these changes, so they should be good for review whenever is convenient for you. (And apologies for the size of the change, it was a bigger change than anticipated even if ~2/3 of it is test code).
A couple follow-up notes after finishing my implementation:
- I realized that quietly() for deletes is the same as idempotent(), but I still inherited the Idempotentable interface in DeleteBuilder and reused the quietly() functionality to be consistent with Create and SetData.
- I didn't implement compareAndDelete or compareAndSetData since those aren't as widely useful as the main Create/SetData/Delete. I could do so in a new PR if desired.
- I did not add idempotent operations to the x-async framework, since this change was already getting large and i'm not sure what the x-async framework is used for. I could do that in a new PR if desired.
- I did not change any of the higher-level recipes to use idempotent operations to simplify their implementation. I could do this in a new PR if desired.
- I did not add idempotent operations to transactions, since I don't think it makes sense to do so.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-770032505
@cammckenzie @eolivelli I think I addressed all of your comments.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745344702
> @jmslocum16 the basic interface looks OK to me, as does the idempotent logic.
>
> Just to confirm my understanding of your solution, your plan is to still have the connection loss exception propagate up to the client code, and they will retry create / update commands with the idempotent flag set, which will succeed if the versions / data match?
Hi Cam, thanks for the quick response!
I'm glad I submitted this patch early - having the client retry was actually not the intended behavior. I had assumed the retry loop inside pathInForeground would handle retrying upon connection loss. It sounds like my assumptions were not correct there and i need to dig more into how retrying works, to make sure the client doesn't have to explicitly retry, and write some tests to validate that behavior.
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r568269227
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -1147,12 +1235,35 @@ public String call() throws Exception
if ( setDataIfExists )
{
Stat setStat = client.getZooKeeper().setData(path, data, setDataIfExistsVersion);
- if(storingStat != null)
+ if ( storingStat != null )
{
DataTree.copyStat(setStat, storingStat);
}
createdPath = path;
}
+ else if ( idempotent && !createMode.isSequential() )
Review comment:
ditto warning suggestion here, or we can check whenever an operation triggered.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -642,6 +657,10 @@ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && setDataIfExists
{
backgroundSetData(client, operationAndData, operationAndData.getData().getPath(), backgrounding);
}
+ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && idempotent && !createMode.isSequential() )
Review comment:
Can we log a warning when user set `idempotent` with sequential node?
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -87,6 +93,8 @@ public CreateBuilderImpl(CuratorFrameworkImpl client, CreateMode createMode, Bac
this.createParentsAsContainers = createParentsAsContainers;
this.compress = compress;
this.setDataIfExists = setDataIfExists;
+ // TODO implement this in CURATOR-589
Review comment:
remove before merging
##########
File path: curator-examples/src/main/java/framework/CrudExamples.java
##########
@@ -119,6 +145,24 @@ public static void guaranteedDelete(CuratorFramework client, String path) t
client.delete().guaranteed().forPath(path);
}
+ public static void idempotentDelete(CuratorFramework client, String path, int currentVersion) throws Exception
Review comment:
nit: why not `deleteIdempotent`
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -210,6 +231,48 @@ public Stat forPath(String path) throws Exception
return this;
}
+ private void backgroundCheckIdempotent(final CuratorFrameworkImpl client, final OperationAndData<PathAndBytes> mainOperationAndData, final String path, final Backgrounding backgrounding)
+ {
+ final AsyncCallback.DataCallback dataCallback = new AsyncCallback.DataCallback()
+ {
+ @Override
+ public void processResult(int rc, String path, Object ctx, byte[] data, Stat stat)
+ {
+ if ( rc == KeeperException.Code.OK.intValue() )
+ {
+ if ( failNextIdempotentCheckForTesting )
+ {
+ failNextIdempotentCheckForTesting = false;
+ rc = KeeperException.Code.CONNECTIONLOSS.intValue();
+ }
+ else if ( !idempotentSetMatches(stat.getVersion(), mainOperationAndData.getData().getData(), data) )
+ {
+ rc = KeeperException.Code.BADVERSION.intValue();
+ }
+ }
+ CuratorEvent event = new CuratorEventImpl(client, CuratorEventType.SET_DATA, rc, path, null, ctx, stat, null, null, null, null, null);
Review comment:
```suggestion
final CuratorEvent event = new CuratorEventImpl(client, CuratorEventType.SET_DATA, rc, path, null, ctx, stat, null, null, null, null, null);
```
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -51,6 +63,8 @@ public SetDataBuilderImpl(CuratorFrameworkImpl client, Backgrounding backgroundi
this.backgrounding = backgrounding;
this.version = version;
this.compress = compress;
+ // TODO implement this in CURATOR-589
Review comment:
ditto removal suggestion here.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -36,13 +39,22 @@
private Backgrounding backgrounding;
private int version;
private boolean compress;
+ private boolean idempotent;
Review comment:
ditto default `false` suggest here.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -57,12 +57,17 @@
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
+ private boolean idempotent;
Review comment:
how about just set a default `false` here?
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-770933464
Rerun travis
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-781418501
Merged to master branch !
thank you @jmslocum16
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-770166459
I will review again on Monday, thanks for the update
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-770907813
> Looks good to me now, not sure why Travis is complaining, I built locally and ran all tests.
@cammckenzie I also ran tests locally and it looked good to me, and travis passed on the last version before I addressed the comments. It looked like the failure was something unrelated:
`
[ERROR] Error occurred in starting fork, check output in log
[ERROR] Process Exit Code: 14
[ERROR] Crashed tests:
[ERROR] org.apache.curator.framework.imps.TestReadOnly
[ERROR] at org.apache.maven.plugin.surefire.booterclient.ForkStarter.awaitResultsDone(ForkStarter.java:532)
....
[ERROR] Caused by: org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?`
What is the expected procedure here? Rerun travis or look into the error more?
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745373676
@cammckenzie I added additional testing and comments that hopefully help to clarify the intended behavior. It appears that TestCreate uses the RetryOnce retry policy, so CreateBuilder is doing one automatic retry, which is sufficient to demonstrate that it successfully retries on ConnectionLoss.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r570598006
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -642,6 +657,10 @@ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && setDataIfExists
{
backgroundSetData(client, operationAndData, operationAndData.getData().getPath(), backgrounding);
}
+ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && idempotent && !createMode.isSequential() )
Review comment:
Actually, after thinking about this some more, and reading the zookeeper documentation, I don't think it's possible to get a NODE_EXISTS exception with sequential creates (which makes sense now that I think about it):
https://zookeeper.apache.org/doc/r3.3.3/api/org/apache/zookeeper/ZooKeeper.html#create(java.lang.String,%20byte[],%20java.util.List,%20org.apache.zookeeper.CreateMode)
So I just removed the check for createMode.isSequential() entirely.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 edited a comment on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 edited a comment on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-770907813
> Looks good to me now, not sure why Travis is complaining, I built locally and ran all tests.
@cammckenzie I also ran tests locally and it looked good to me, and travis passed on the last version before I addressed the comments. It looked like the failure was something unrelated:
`
[ERROR] Error occurred in starting fork, check output in log
[ERROR] Process Exit Code: 14
[ERROR] Crashed tests:
[ERROR] org.apache.curator.framework.imps.TestReadOnly
[ERROR] at org.apache.maven.plugin.surefire.booterclient.ForkStarter.awaitResultsDone(ForkStarter.java:532)
....
[ERROR] Caused by: org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?`
What is the expected procedure here? Rerun travis or look into the error more?
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-780419404
@TisonKun @eolivelli any further comments. If you're happy, I will merge.
cheers
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745552963
@jmslocum16 I probably worded my question poorly. The retry policy will certainly execute a retry on connection loss, but if that retry fails after the configured number of attempts, the error will propagate back to the client which must then do something with it. I was thinking that perhaps you were suggesting something like the Curator guaranteed deletes, which will retry indefinitely until the zNode in question no longer exists (this approach of retrying in the background indefinitely obviously won't work for create / update cases).
I will try and take a look at your updates today.
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-766454397
Thanks @jmslocum16, will try and take a look in the next few day.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r572482518
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -569,7 +583,7 @@ public String forPath(final String givenPath, byte[] data) throws Exception
data = client.getCompressionProvider().compress(givenPath, data);
}
- final String adjustedPath = adjustPath(client.fixForNamespace(givenPath, createMode.isSequential()));
+ final String adjustedPath = adjustPath(client.fixForNamespace(givenPath, createMode.isnSequential()));
Review comment:
oh, yeah :( fixed
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r567004363
##########
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:
created https://issues.apache.org/jira/browse/CURATOR-589
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-781643211
Thanks to everyone for your helpful comments!
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-771211557
@Randgalt the problem doesn't seem to be the test though, from the log it looks like the JVM is crashing in some capacity. The tests have been stable for me locally since you put some work into fixing them a while ago.
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
Randgalt commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-771208994
> Looks like Travis failed again, I have zero Travis experience unfortunately. Is this a memory issue? Or something else?
> [ERROR] Error occurred in starting fork, check output in log
Our tests are extremely flakey. They always have been.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745346475
Also should I be worried about the CI failing for jdk8 but not jdk11?
It looks unrelated but I'd want to make sure - https://travis-ci.com/github/apache/curator/jobs/459964173
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r566630026
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -210,6 +230,47 @@ public Stat forPath(String path) throws Exception
return this;
}
+ private void backgroundCheckIdempotent(final CuratorFrameworkImpl client, final OperationAndData<PathAndBytes> mainOperationAndData, final String path, final Backgrounding backgrounding)
+ {
+ final AsyncCallback.DataCallback dataCallback = new AsyncCallback.DataCallback()
+ {
+ @Override
+ public void processResult(int rc, String path, Object ctx, byte[] data, Stat stat)
+ {
+ if ( rc == KeeperException.Code.OK.intValue() )
+ {
+ if ( failNextIdempotentCheckForTesting )
+ {
+ failNextIdempotentCheckForTesting = false;
+ rc = KeeperException.Code.CONNECTIONLOSS.intValue();
+ }
+ else if ( !idempotentSetMatches(stat.getVersion(), mainOperationAndData.getData().getData(), data) )
+ {
+ rc = KeeperException.Code.BADVERSION.intValue();
+ }
+ }
+ CuratorEvent event = new CuratorEventImpl(client, CuratorEventType.SET_DATA, rc, path, null, ctx, stat, null, null, null, null, null);
+ client.processBackgroundOperation(mainOperationAndData, event);
+ }
+ };
+ BackgroundOperation<PathAndBytes> operation = new BackgroundOperation<PathAndBytes>()
+ {
+ @Override
+ public void performBackgroundOperation(OperationAndData<PathAndBytes> op) throws Exception
+ {
+ try
+ {
+ client.getZooKeeper().getData(path, false, dataCallback, backgrounding.getContext());
+ }
+ catch ( KeeperException e )
+ {
+ // ignore
Review comment:
log ?
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -807,6 +826,53 @@ public void performBackgroundOperation(OperationAndData<PathAndBytes> op) throws
client.queueOperation(new OperationAndData<>(operation, null, null, null, null, null));
}
+ private void backgroundCheckIdempotent(final CuratorFrameworkImpl client, final OperationAndData<PathAndBytes> mainOperationAndData, final String path, final Backgrounding backgrounding)
+ {
+ final AsyncCallback.DataCallback dataCallback = new AsyncCallback.DataCallback()
+ {
+ @Override
+ public void processResult(int rc, String path, Object ctx, byte[] data, Stat stat)
+ {
+ if ( rc == KeeperException.Code.NONODE.intValue() )
+ {
+ client.queueOperation(mainOperationAndData); // try to create it again
+ }
+ else
+ {
+ if ( rc == KeeperException.Code.OK.intValue() )
+ {
+ if ( failNextIdempotentCheckForTesting )
+ {
+ failNextIdempotentCheckForTesting = false;
+ rc = KeeperException.Code.CONNECTIONLOSS.intValue();
+ }
+ else if ( !IdempotentUtils.matches(0, mainOperationAndData.getData().getData(), stat.getVersion(), data) )
+ {
+ rc = KeeperException.Code.NODEEXISTS.intValue();
+ }
+ }
+ sendBackgroundResponse(rc, path, ctx, path, stat, mainOperationAndData);
+ }
+ }
+ };
+ BackgroundOperation<PathAndBytes> operation = new BackgroundOperation<PathAndBytes>()
+ {
+ @Override
+ public void performBackgroundOperation(OperationAndData<PathAndBytes> op) throws Exception
+ {
+ try
+ {
+ client.getZooKeeper().getData(path, false, dataCallback, backgrounding.getContext());
+ }
+ catch ( KeeperException e )
+ {
+ // ignore
Review comment:
log ?
##########
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:
It should be fine to make the upgrade
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/IdempotentUtils.java
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.imps;
+
+/*
+ * Utilties Class for idempotent operatons.
+ */
+class IdempotentUtils
+{
+
+ /**
+ * Returns whether the version and data currently in the node match what would be expected in the idempotent retry case.
+ */
+ static boolean matches(int expectedVersion, byte[] expectedData, int actualVersion, byte[] actualData)
+ {
+ return expectedVersion == actualVersion && matches(expectedData, actualData);
+ }
+
+ /**
+ * Returns whether the data currently in the node matches what would be expected in the idempotent retry case (ignoring version).
+ */
+ static boolean matches(byte[] expectedData, byte[] actualData)
+ {
+ if ( expectedData.length != actualData.length )
Review comment:
can we use java.util.Arrays.equals ?
##########
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:
can we link a JIRA ?
##########
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:
can we please remove TODO and create an issue ?
or we can leave here the TODO but put the link to the JIRA
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-771196143
Looks like Travis failed again, I have zero Travis experience unfortunately. Is this a memory issue? Or something else?
[ERROR] Error occurred in starting fork, check output in log
[ERROR] Process Exit Code: 14
[ERROR] Crashed tests:
[ERROR] org.apache.curator.framework.recipes.cache.TestCuratorCacheEdges
[ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: ExecutionException The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
[ERROR] Command was /bin/sh -c cd /home/travis/build/apache/curator/curator-recipes && /usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java -jar /home/travis/build/apache/curator/curator-recipes/target/surefire/surefirebooter7288833367410099534.jar /home/travis/build/apache/curator/curator-recipes/target/surefire 2021-02-01T20-48-51_335-jvmRun1 surefire753304824818451703tmp surefire_618215176770182383564tmp
[ERROR] Error occurred in starting fork, check output in log
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745568796
Ah @cammckenzie I see what you meant! Apologies for misinterpreting. That's correct. If the retry limit is exceeded, the exception will propagate back to the client. I agree that for creates/updates, retrying indefinitely in the background wouldn't make sense, and I think propagating the exception and having the client decide to retry or not is the best approach.
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r572458837
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -569,7 +583,7 @@ public String forPath(final String givenPath, byte[] data) throws Exception
data = client.getCompressionProvider().compress(givenPath, data);
}
- final String adjustedPath = adjustPath(client.fixForNamespace(givenPath, createMode.isSequential()));
+ final String adjustedPath = adjustPath(client.fixForNamespace(givenPath, createMode.isnSequential()));
Review comment:
isnSequential is presumably a typo?
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-767825853
@jmslocum16, looks good. Some minor comments, but looks pretty close to mergeable to me.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 closed pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 closed pull request #376:
URL: https://github.com/apache/curator/pull/376
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r568269227
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -1147,12 +1235,35 @@ public String call() throws Exception
if ( setDataIfExists )
{
Stat setStat = client.getZooKeeper().setData(path, data, setDataIfExistsVersion);
- if(storingStat != null)
+ if ( storingStat != null )
{
DataTree.copyStat(setStat, storingStat);
}
createdPath = path;
}
+ else if ( idempotent && !createMode.isSequential() )
Review comment:
ditto warning suggestion here, or we can check whenever an operation triggered.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -642,6 +657,10 @@ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && setDataIfExists
{
backgroundSetData(client, operationAndData, operationAndData.getData().getPath(), backgrounding);
}
+ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && idempotent && !createMode.isSequential() )
Review comment:
Can we log a warning when user set `idempotent` with sequential node?
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -87,6 +93,8 @@ public CreateBuilderImpl(CuratorFrameworkImpl client, CreateMode createMode, Bac
this.createParentsAsContainers = createParentsAsContainers;
this.compress = compress;
this.setDataIfExists = setDataIfExists;
+ // TODO implement this in CURATOR-589
Review comment:
remove before merging
##########
File path: curator-examples/src/main/java/framework/CrudExamples.java
##########
@@ -119,6 +145,24 @@ public static void guaranteedDelete(CuratorFramework client, String path) t
client.delete().guaranteed().forPath(path);
}
+ public static void idempotentDelete(CuratorFramework client, String path, int currentVersion) throws Exception
Review comment:
nit: why not `deleteIdempotent`
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -210,6 +231,48 @@ public Stat forPath(String path) throws Exception
return this;
}
+ private void backgroundCheckIdempotent(final CuratorFrameworkImpl client, final OperationAndData<PathAndBytes> mainOperationAndData, final String path, final Backgrounding backgrounding)
+ {
+ final AsyncCallback.DataCallback dataCallback = new AsyncCallback.DataCallback()
+ {
+ @Override
+ public void processResult(int rc, String path, Object ctx, byte[] data, Stat stat)
+ {
+ if ( rc == KeeperException.Code.OK.intValue() )
+ {
+ if ( failNextIdempotentCheckForTesting )
+ {
+ failNextIdempotentCheckForTesting = false;
+ rc = KeeperException.Code.CONNECTIONLOSS.intValue();
+ }
+ else if ( !idempotentSetMatches(stat.getVersion(), mainOperationAndData.getData().getData(), data) )
+ {
+ rc = KeeperException.Code.BADVERSION.intValue();
+ }
+ }
+ CuratorEvent event = new CuratorEventImpl(client, CuratorEventType.SET_DATA, rc, path, null, ctx, stat, null, null, null, null, null);
Review comment:
```suggestion
final CuratorEvent event = new CuratorEventImpl(client, CuratorEventType.SET_DATA, rc, path, null, ctx, stat, null, null, null, null, null);
```
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -51,6 +63,8 @@ public SetDataBuilderImpl(CuratorFrameworkImpl client, Backgrounding backgroundi
this.backgrounding = backgrounding;
this.version = version;
this.compress = compress;
+ // TODO implement this in CURATOR-589
Review comment:
ditto removal suggestion here.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -36,13 +39,22 @@
private Backgrounding backgrounding;
private int version;
private boolean compress;
+ private boolean idempotent;
Review comment:
ditto default `false` suggest here.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -57,12 +57,17 @@
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
+ private boolean idempotent;
Review comment:
how about just set a default `false` here?
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
TisonKun commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-780448676
> @TisonKun @eolivelli any further comments. If you're happy, I will merge.
> cheers
@cammckenzie already given my lgtm.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-769429852
> @jmslocum16, looks good. Some minor comments, but looks pretty close to mergeable to me.
Thanks @cammckenzie! You mentioned some minor comments, but I can't find any comments you left. Is there something with the MR UI I'm missing, or are there more comments you're planning on adding?
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745731111
This looks like a good start to me @jmslocum16. Don't worry about the CI failing for now, some of the unit tests are a bit flaky, will look in more detail before we merge.This looks like a good start to me @jmslocum16. Don't worry about the CI failing for now, some of the unit tests are a bit flaky, will look in more detail before we merge.
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by 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
[GitHub] [curator] jmslocum16 commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r570598006
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -642,6 +657,10 @@ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && setDataIfExists
{
backgroundSetData(client, operationAndData, operationAndData.getData().getPath(), backgrounding);
}
+ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && idempotent && !createMode.isSequential() )
Review comment:
Actually, after thinking about this some more, and reading the zookeeper documentation, I don't think it's possible to get a NODE_EXISTS exception with sequential creates (which makes sense now that I think about it):
https://zookeeper.apache.org/doc/r3.3.3/api/org/apache/zookeeper/ZooKeeper.html#create(java.lang.String,%20byte[],%20java.util.List,%20org.apache.zookeeper.CreateMode)
So I just removed the check for createMode.isSequential() entirely.
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-770459003
Looks good to me now, not sure why Travis is complaining, I built locally and ran all tests.
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-775811953
Looks good to me now. Any further comments from anyone else?
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: WIP: Starting with foreground idempotent create
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-745019125
@jmslocum16 the basic interface looks OK to me, as does the idempotent logic.
Just to confirm my understanding of your solution, your plan is to still have the connection loss exception propagate up to the client code, and they will retry create / update commands with the idempotent flag set, which will succeed if the versions / data match?
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli closed pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #376:
URL: https://github.com/apache/curator/pull/376
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r573019771
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -642,6 +657,10 @@ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && setDataIfExists
{
backgroundSetData(client, operationAndData, operationAndData.getData().getPath(), backgrounding);
}
+ else if ( (rc == KeeperException.Code.NODEEXISTS.intValue()) && idempotent && !createMode.isSequential() )
Review comment:
you're right. sequential always succeeds.
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-776239344
Whoops I accidentally clicked the "close PR" button.
Thanks a bunch for reviewing @cammckenzie @TisonKun and @eolivelli!
I'll await any new comments from Enrico, and I addressed your quality comments Tison.
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli edited a comment on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-781418501
Merged to master branch !
thank you @jmslocum16
see https://github.com/apache/curator/commit/d5666ab9ca22f45b905d6ffbf2b33fa30b38e15e
----------------------------------------------------------------
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
[GitHub] [curator] jmslocum16 commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
jmslocum16 commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r566506371
##########
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:
Yeah, I changed this because i was getting build errors trying to build curator locally.
This was the recommended fix I found, and it did fix it for me. I am also not a maven expert so if there is a different or better way to fix it I'm happy to change it.
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #376:
URL: https://github.com/apache/curator/pull/376#issuecomment-769469464
@jmslocum16 Forgot to complete my review, so the comments were all in a pending state, they should be there now.
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on a change in pull request #376: CURATOR-584: Added fault tolerant idempotent Create, SetData and Delete operations
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #376:
URL: https://github.com/apache/curator/pull/376#discussion_r573012387
##########
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;
Review comment:
ditto
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
##########
@@ -74,6 +79,7 @@
createParentsAsContainers = false;
compress = false;
setDataIfExists = false;
+ idempotent = false;
Review comment:
if we have a default value, then this is redundant.
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -36,13 +39,22 @@
private Backgrounding backgrounding;
private int version;
private boolean compress;
+ private boolean idempotent = false;
+
+ @VisibleForTesting
+ boolean failNextSetForTesting = false;
+ @VisibleForTesting
+ boolean failBeforeNextSetForTesting = false;
+ @VisibleForTesting
+ boolean failNextIdempotentCheckForTesting = false;
SetDataBuilderImpl(CuratorFrameworkImpl client)
{
this.client = client;
backgrounding = new Backgrounding();
version = -1;
compress = false;
+ idempotent = false;
Review comment:
ditto
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/SetDataBuilderImpl.java
##########
@@ -51,6 +63,7 @@ public SetDataBuilderImpl(CuratorFrameworkImpl client, Backgrounding backgroundi
this.backgrounding = backgrounding;
this.version = version;
this.compress = compress;
+ this.idempotent = false;
Review comment:
ditto
----------------------------------------------------------------
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