simplifies concurrency in ZooSession and ZooCache #5303
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Made a few changes to how ZooCache and ZooSession handle concurrency in order to make it easier to reason about the code. ZooCache and ZooSession had code that used Atomics to do computations in addition to synchronized blocks. This adds complexity w/o an apparent benefit. Stopped doing the computations in both places and moved all computations in to the synchronized block. ZooSession has a counter that was used by ZooCache to know when zookeeper changed. ZooSession updated this counter independently from updating the zookeeper reference and ZooCache read the refrence and counter at different times. Reorganized the code so that read and write of the counter and zookeeper are done at the same times by ZooSession and ZooCache. ZooSession.close() could run conucurrently with the code that created new ZooKeeper objects in ZooSession. Made the close method synchronized and tweaked how the closed variable is checked.
cshannon
approved these changes
Feb 7, 2025
| @@ -348,7 +377,13 @@ public ZooReaderWriter asReaderWriter() { | |||
| * @return connection counter | |||
| */ | |||
| public long getConnectionCounter() { | |||
There was a problem hiding this comment.
Should this method also be marked as synchronized like the others?
There was a problem hiding this comment.
This method does not need to be synchronized as it reads the latest from a volatile. Could possibly make it synchronized and further simplify the code, but that would require synchronizing a few other methods. The zookeeper ref and zk counter need to be read and written as one. That is done by the way the current atomic ref is structured. It could also be done by having two instance variables and always reading/writing either of those variables in a sync block. Since the synchronization would cascade for making two instance variables will leave this as-is for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Made a few changes to how ZooCache and ZooSession handle concurrency in order to make it easier to reason about the code.
ZooCache and ZooSession had code that used Atomics to do computations in addition to synchronized blocks. This adds complexity w/o an apparent benefit. Stopped doing the computations in both places and moved all computations in to the synchronized block.
ZooSession has a counter that was used by ZooCache to know when zookeeper changed. ZooSession updated this counter independently from updating the zookeeper reference and ZooCache read the refrence and counter at different times. Reorganized the code so that read and write of the counter and zookeeper are done at the same times by ZooSession and ZooCache.
ZooSession.close() could run conucurrently with the code that created new ZooKeeper objects in ZooSession. Made the close method synchronized and tweaked how the closed variable is checked.