gianm commented on code in PR #14616:
URL: https://github.com/apache/druid/pull/14616#discussion_r1268383746
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -789,15 +789,20 @@ private CustomSettableFuture(
public void resolve()
{
- synchronized (statusRefs) {
+ synchronized (requestStatusesLock) {
if (isDone()) {
return;
}
- List<DataSegmentChangeRequestAndStatus> result = new
ArrayList<>(statusRefs.size());
- statusRefs.forEach(
- (request, statusRef) -> result.add(new
DataSegmentChangeRequestAndStatus(request, statusRef.get()))
- );
+ final List<DataSegmentChangeRequestAndStatus> result = new
ArrayList<>(statusRefs.size());
+ statusRefs.forEach((request, statusRef) -> {
+ // Remove complete statuses from the cache
Review Comment:
When is this `resolve` method called exactly? Is it possible that these
invalidations are too late, i.e. is it still possible that an even faster
load->drop->load would be racey?
Put another way: would it make sense to invalidate the cache on receipt of
the drop, rather than here?
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -789,15 +789,20 @@ private CustomSettableFuture(
public void resolve()
{
- synchronized (statusRefs) {
+ synchronized (requestStatusesLock) {
Review Comment:
I am trying to understand what the implication is of switching from
`statusRefs` to `requestStatusesLock` is. However, this file doesn't use
`@GuardedBy` and there isn't a comment on `requestStatusesLock` explaining how
it's supposed to be used. Would you be able to add one or both of those, to
make it easier to understand the synchronization logic?
--
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]
