findingrish commented on code in PR #14985:
URL: https://github.com/apache/druid/pull/14985#discussion_r1375248637
##########
server/src/main/java/org/apache/druid/client/SegmentLoadInfo.java:
##########
@@ -59,6 +61,15 @@ public ImmutableSegmentLoadInfo toImmutableSegmentLoadInfo()
return new ImmutableSegmentLoadInfo(segment, servers);
}
+ /**
+ * Randomly return one server from the sets of {@code servers}
+ */
+ public DruidServerMetadata pickOne()
+ {
+ synchronized (this) {
Review Comment:
> hmm, this still isn't quite right i think,
yes, I must synchronise the other methods as well.
Or, should we continue to use the concurrent set and synchronise only `add`,
`remove` and `pickOne`?
> So I think we can just iterate to pick one...
`Iterators.get(servers.iterator(),
ThreadLocalRandom.current().nextInt(servers.size()));`
Why do you suggest that we don't need synchronisation for this method.
The `servers` size can change while iteration, if the server is removed then
it can potentially result in `IndexOutOfBoundsException`.
> but if we do, then something should probably be pushing a computation into
this class to do the thing so that we can do the whole operation under lock...
can you please clarify this statement?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
