You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by dragonsinth <gi...@git.apache.org> on 2014/10/04 04:06:38 UTC
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
GitHub user dragonsinth opened a pull request:
https://github.com/apache/curator/pull/47
CURATOR-151: SharedValue/SharedCount API update
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dragonsinth/curator CURATOR-151
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/47.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 #47
----
commit 0bc73191ba0fce5fb28894f8e4124d9d42655b37
Author: Scott Blum <sc...@squareup.com>
Date: 2014-10-04T02:04:36Z
CURATOR-151: SharedValue/SharedCount API update
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/47#issuecomment-59428448
Thanks! Good to merge then?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18427001
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/VersionedValue.java ---
@@ -32,7 +32,7 @@
* @param version the version
* @param value the value (cannot be null)
*/
- public VersionedValue(int version, T value)
+ VersionedValue(int version, T value)
--- End diff --
made non-public to force clients to use a previously-retrieved value instead of trying to construct one from thin air
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18426992
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -117,10 +117,8 @@ public void setValue(byte[] newValue) throws Exception
{
Preconditions.checkState(state.get() == State.STARTED, "not started");
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().forPath(path, newValue);
-
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
+ Stat result = client.setData().forPath(path, newValue);
+ updateValue(result.getVersion(), Arrays.copyOf(newValue, newValue.length));
--- End diff --
the oldVersion+1 thing seemed a little squirrelly to me
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18558295
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -129,29 +127,19 @@ public void setValue(byte[] newValue) throws Exception
* value is updated. i.e. if the value is not successful you can get the updated value
* by calling {@link #getValue()}.
*
+ * @deprecated use {@link #trySetValue(VersionedValue, byte[])} for stronger atomicity
+ * guarantees. Even if this object's internal state is up-to-date, the caller has no way to
+ * ensure that they've read the most recently seen value.
+ *
* @param newValue the new value to attempt
* @return true if the change attempt was successful, false if not. If the change
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
+ @Deprecated
public boolean trySetValue(byte[] newValue) throws Exception
{
- Preconditions.checkState(state.get() == State.STARTED, "not started");
-
- try
- {
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().withVersion(localCopy.getVersion()).forPath(path, newValue);
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
- return true;
- }
- catch ( KeeperException.BadVersionException ignore )
- {
- // ignore
- }
-
- readValue();
- return false;
+ return trySetValue(currentValue.get(), newValue);
}
--- End diff --
I don't like this because it changes the behavior of the class. If anyone was depending on old behavior their code will break.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/47#issuecomment-59415931
No - I'll trust you on that sync optimization.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18558309
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -165,14 +153,20 @@ public boolean trySetValue(byte[] newValue) throws Exception
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
- public boolean trySetValue(VersionedValue<byte[]> newValue) throws Exception
+ public boolean trySetValue(VersionedValue<byte[]> previous, byte[] newValue) throws Exception
{
Preconditions.checkState(state.get() == State.STARTED, "not started");
+ VersionedValue<byte[]> current = currentValue.get();
+ if ( previous.getVersion() != current.getVersion() || !Arrays.equals(previous.getValue(), current.getValue()) )
+ {
+ return false;
+ }
+
try
{
- client.setData().withVersion(newValue.getVersion()).forPath(path, newValue.getValue());
- currentValue.set(new VersionedValue<byte[]>(newValue.getVersion() + 1, Arrays.copyOf(newValue.getValue(), newValue.getValue().length)));
+ Stat result = client.setData().withVersion(previous.getVersion()).forPath(path, newValue);
+ updateValue(result.getVersion(), Arrays.copyOf(newValue, newValue.length));
--- End diff --
If we're going to add this behavior I'd change the name of this method so it's clear about the behavior.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18426996
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -225,11 +238,11 @@ public void close() throws IOException
listeners.clear();
}
- private synchronized void readValue() throws Exception
+ private void readValue() throws Exception
--- End diff --
we might still want to synchronize to avoid multiple concurrent calls to client.getData(), but we don't need it for correctness anymore. WDYT?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18621853
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -184,6 +178,25 @@ public boolean trySetValue(VersionedValue<byte[]> newValue) throws Exception
return false;
}
+ private void updateValue(int version, byte[] bytes)
+ {
+ while (true)
+ {
+ VersionedValue<byte[]> current = currentValue.get();
+ if (current.getVersion() >= version)
+ {
+ // A newer version was concurrently set.
+ return;
+ }
+ if ( currentValue.compareAndSet(current, new VersionedValue<byte[]>(version, bytes)) )
+ {
+ // Successfully set.
+ return;
+ }
+ // Lost a race, retry.
+ }
+ }
+
/**
--- End diff --
This is the new behavior as I see it. It loops until it successfully updates the value. Right?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18874097
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -129,29 +127,19 @@ public void setValue(byte[] newValue) throws Exception
* value is updated. i.e. if the value is not successful you can get the updated value
* by calling {@link #getValue()}.
*
+ * @deprecated use {@link #trySetValue(VersionedValue, byte[])} for stronger atomicity
+ * guarantees. Even if this object's internal state is up-to-date, the caller has no way to
+ * ensure that they've read the most recently seen value.
+ *
* @param newValue the new value to attempt
* @return true if the change attempt was successful, false if not. If the change
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
+ @Deprecated
public boolean trySetValue(byte[] newValue) throws Exception
{
- Preconditions.checkState(state.get() == State.STARTED, "not started");
-
- try
- {
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().withVersion(localCopy.getVersion()).forPath(path, newValue);
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
- return true;
- }
- catch ( KeeperException.BadVersionException ignore )
- {
- // ignore
- }
-
- readValue();
- return false;
+ return trySetValue(currentValue.get(), newValue);
}
--- End diff --
For more reference, check out [5 Things You Didn’t Know About Synchronization in Java and Scala](http://www.takipiblog.com/5-things-you-didnt-know-about-synchronization-in-java-and-scala/).
"If the CAS fails the JVM will perform one round of spin locking where the thread parks to effectively put it to sleep between retrying the CAS. If these initial attempts fail (signaling a fairly higher level of contention for the lock) the thread will move itself to a blocked state and enqueue itself in the list of threads vying for the lock and begin a series of spin locks."
I'm essentially doing in code what the synchronization did internally. That's why I didn't have to rewrite any test code other than for the API change on the new method that was recently added.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18800916
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -129,29 +127,19 @@ public void setValue(byte[] newValue) throws Exception
* value is updated. i.e. if the value is not successful you can get the updated value
* by calling {@link #getValue()}.
*
+ * @deprecated use {@link #trySetValue(VersionedValue, byte[])} for stronger atomicity
+ * guarantees. Even if this object's internal state is up-to-date, the caller has no way to
+ * ensure that they've read the most recently seen value.
+ *
* @param newValue the new value to attempt
* @return true if the change attempt was successful, false if not. If the change
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
+ @Deprecated
public boolean trySetValue(byte[] newValue) throws Exception
{
- Preconditions.checkState(state.get() == State.STARTED, "not started");
-
- try
- {
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().withVersion(localCopy.getVersion()).forPath(path, newValue);
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
- return true;
- }
- catch ( KeeperException.BadVersionException ignore )
- {
- // ignore
- }
-
- readValue();
- return false;
+ return trySetValue(currentValue.get(), newValue);
}
--- End diff --
No, not really. The old version used a synchronized block. This version uses a compare-and-set loop. It's only a difference in how concurrency is handled, it's not an algorithm change. For all practical purposes, the CAS loop almost never executes more than once.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/47#issuecomment-59414203
Anything else I can do to push this PR forward?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18558516
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -129,29 +127,19 @@ public void setValue(byte[] newValue) throws Exception
* value is updated. i.e. if the value is not successful you can get the updated value
* by calling {@link #getValue()}.
*
+ * @deprecated use {@link #trySetValue(VersionedValue, byte[])} for stronger atomicity
+ * guarantees. Even if this object's internal state is up-to-date, the caller has no way to
+ * ensure that they've read the most recently seen value.
+ *
* @param newValue the new value to attempt
* @return true if the change attempt was successful, false if not. If the change
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
+ @Deprecated
public boolean trySetValue(byte[] newValue) throws Exception
{
- Preconditions.checkState(state.get() == State.STARTED, "not started");
-
- try
- {
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().withVersion(localCopy.getVersion()).forPath(path, newValue);
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
- return true;
- }
- catch ( KeeperException.BadVersionException ignore )
- {
- // ignore
- }
-
- readValue();
- return false;
+ return trySetValue(currentValue.get(), newValue);
}
--- End diff --
Hmm, so when I wrote this code I was 99% sure this would NOT be a change to the existing behavior. In other words, if you were to inline the 2-arg version into the 1-arg version, I think you get (essentially) identical code. I was just trying to DRY this up. What do you see being different?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18659175
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -184,6 +178,25 @@ public boolean trySetValue(VersionedValue<byte[]> newValue) throws Exception
return false;
}
+ private void updateValue(int version, byte[] bytes)
+ {
+ while (true)
+ {
+ VersionedValue<byte[]> current = currentValue.get();
+ if (current.getVersion() >= version)
+ {
+ // A newer version was concurrently set.
+ return;
+ }
+ if ( currentValue.compareAndSet(current, new VersionedValue<byte[]>(version, bytes)) )
+ {
+ // Successfully set.
+ return;
+ }
+ // Lost a race, retry.
+ }
+ }
+
/**
--- End diff --
Let me elaborate a little on the race condition this solves.
Imagine the current value is (1, A) you have one thread calling trySetValue(B) another calling trySetValue(C) at the same time. Imagine thread 1 succeeds in making the server call first and updating the server value to (2, B) and immediately afterwards thread 2 succeeds in calling the server and updating the server value to (3, C). But to due to thread scheduling or what have you, both threads actually enter updateValue() at the same time. How do we ensure that the correct value ends up in currentValue when both threads exit updateValue so that the client has the correct server state?
That's what the compare-and-set loop solves. Imagine both threads enter the loop and initially read current as (1, A). Then they both try to compare and set (1, A) to either (2, B) or (3, C). Only one of them can win the race. If (2, B) gets set, then the first thread will return and the second thread will try again, and this time succeed in updating (2, B) -> (3, C). If (3, C) gets set then the second thread will return, and the first thread will try again. But this time it will exit out because it will see that someone else already set a higher-versioned value.
This actually solves several potential races, between threads calling setValue/trySetValue at the same time and also any threads that could be calling readValue() at the same time (watcher, failed trySetValue(), or even the start() call).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/47
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18558577
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -165,14 +153,20 @@ public boolean trySetValue(byte[] newValue) throws Exception
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
- public boolean trySetValue(VersionedValue<byte[]> newValue) throws Exception
+ public boolean trySetValue(VersionedValue<byte[]> previous, byte[] newValue) throws Exception
{
Preconditions.checkState(state.get() == State.STARTED, "not started");
+ VersionedValue<byte[]> current = currentValue.get();
+ if ( previous.getVersion() != current.getVersion() || !Arrays.equals(previous.getValue(), current.getValue()) )
+ {
+ return false;
+ }
+
try
{
- client.setData().withVersion(newValue.getVersion()).forPath(path, newValue.getValue());
- currentValue.set(new VersionedValue<byte[]>(newValue.getVersion() + 1, Arrays.copyOf(newValue.getValue(), newValue.getValue().length)));
+ Stat result = client.setData().withVersion(previous.getVersion()).forPath(path, newValue);
+ updateValue(result.getVersion(), Arrays.copyOf(newValue, newValue.length));
--- End diff --
I don't think I understand. Which method would you rename, and what kind of name would you think? Do you mean that the 2-arg version should be named something like "compareAndSetValue"?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18557177
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -117,10 +117,8 @@ public void setValue(byte[] newValue) throws Exception
{
Preconditions.checkState(state.get() == State.STARTED, "not started");
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().forPath(path, newValue);
-
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
+ Stat result = client.setData().forPath(path, newValue);
+ updateValue(result.getVersion(), Arrays.copyOf(newValue, newValue.length));
--- End diff --
Yeah I agree.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/47#issuecomment-57891735
@Randgalt
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18633269
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -184,6 +178,25 @@ public boolean trySetValue(VersionedValue<byte[]> newValue) throws Exception
return false;
}
+ private void updateValue(int version, byte[] bytes)
+ {
+ while (true)
+ {
+ VersionedValue<byte[]> current = currentValue.get();
+ if (current.getVersion() >= version)
+ {
+ // A newer version was concurrently set.
+ return;
+ }
+ if ( currentValue.compareAndSet(current, new VersionedValue<byte[]>(version, bytes)) )
+ {
+ // Successfully set.
+ return;
+ }
+ // Lost a race, retry.
+ }
+ }
+
/**
--- End diff --
Ah, gotcha. So this isn't new behavior -- this is just a standard compare-and-set loop that replaces the synchronized keyword. It's not round tripping to the server multiple times. There's still only one attempt to set the server value, and if it fails the calling methods return false. This is just atomically setting currentValue. We could replace the loop with a synchronized block, but performance would be worse.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/47#discussion_r18800636
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java ---
@@ -129,29 +127,19 @@ public void setValue(byte[] newValue) throws Exception
* value is updated. i.e. if the value is not successful you can get the updated value
* by calling {@link #getValue()}.
*
+ * @deprecated use {@link #trySetValue(VersionedValue, byte[])} for stronger atomicity
+ * guarantees. Even if this object's internal state is up-to-date, the caller has no way to
+ * ensure that they've read the most recently seen value.
+ *
* @param newValue the new value to attempt
* @return true if the change attempt was successful, false if not. If the change
* was not successful, {@link #getValue()} will return the updated value
* @throws Exception ZK errors, interruptions, etc.
*/
+ @Deprecated
public boolean trySetValue(byte[] newValue) throws Exception
{
- Preconditions.checkState(state.get() == State.STARTED, "not started");
-
- try
- {
- VersionedValue<byte[]> localCopy = currentValue.get();
- client.setData().withVersion(localCopy.getVersion()).forPath(path, newValue);
- currentValue.set(new VersionedValue<byte[]>(localCopy.getVersion() + 1, Arrays.copyOf(newValue, newValue.length)));
- return true;
- }
- catch ( KeeperException.BadVersionException ignore )
- {
- // ignore
- }
-
- readValue();
- return false;
+ return trySetValue(currentValue.get(), newValue);
}
--- End diff --
The new version loops right? That's different than the old version.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-151: SharedValue/SharedCount API upd...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/47#issuecomment-59428499
Engage merging speed
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---