leventov commented on a change in pull request #7088: Improve parallelism of
zookeeper based segment change processing
URL: https://github.com/apache/incubator-druid/pull/7088#discussion_r277355910
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -183,28 +172,17 @@ public void dropSegment(
final LoadPeonCallback callback
Review comment:
Why did you remove the synchronization in the first place?
The change doesn't quite work without concurrent access documentation.
Concurrent access documentation should go hand-in-hand with the concurrently
safe code. Without the documentation, how would anybody know why
`putIfAbsent()` is used, and if that is enough?
I would not ask you to add documentation if you didn't remove
synchronization and change the concurrency model of this class in the first
place. But since you did, you have to be able to answer "hard" questions about
this class. There is no other way to verify this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
