kfaraz commented on code in PR #14616: URL: https://github.com/apache/druid/pull/14616#discussion_r1273350848
########## server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java: ########## @@ -90,19 +93,46 @@ public class SegmentLoadDropHandler implements DataSegmentChangeHandler private final SegmentManager segmentManager; private final ScheduledExecutorService exec; private final ServerTypeConfig serverTypeConfig; - private final ConcurrentSkipListSet<DataSegment> segmentsToDelete; + private final ConcurrentSkipListSet<DataSegment> segmentsToDrop; private final SegmentCacheManager segmentCacheManager; private volatile boolean started = false; - // Keep history of load/drop request status in a LRU cache to maintain idempotency if same request shows up - // again and to return status of a completed request. Maximum size of this cache must be significantly greater - // than number of pending load/drop requests. so that history is not lost too quickly. - private final Cache<DataSegmentChangeRequest, AtomicReference<Status>> requestStatuses; + /** + * Used to cache the status of a completed load or drop request until it has + * been served to the (Coordinator) client exactly once. + * <p> + * The cache is used as follows: + * <ol> + * <li>An entry with state PENDING is added to the cache upon receiving a + * request to load or drop a segment.</li> + * <li>A duplicate request received at this point is immediately answered with PENDING.</li> + * <li>Once the load/drop finishes, the entry is updated to either SUCCESS or FAILED.</li> + * <li>A duplicate request received at this point is immediately answered with + * SUCCESS or FAILED and the entry is removed from the cache.</li> + * <li>If the first request itself finishes after the load or drop has already + * completed, it is answered with a SUCCESS or FAILED and the entry is removed + * from the cache.</li> + * </ol> + * <p> + * Maximum size of this cache must be significantly greater than the number of Review Comment: > Btw, I am wondering now, what are the scenarios where the Historical might receive multiple copies of the same load or drop command? Do you mean the same action on same segment or different actions on same segment? Both of these can actually happen under normal conditions: #### Same action on same segment Even under normal operation, the Coordinator keeps sending the same request to a Historical until the request finishes. Essentially, the Coordinator sends a batch of requests to the Historical and waits for a response. Historical sends back a response as soon as any _one_ of the requests in this batch (or even a previously submitted batch which is still being tracked) finishes. So chances are that the Coordinator gets back a response with one request having status SUCCESS or FAILED and the others PENDING, because the Historical was still working on them when it completed the HTTP request. The Coordinator then re-sends the requests for which it had received a PENDING status. By then, the Historical might have already finished them and cached their results. #### Different actions on same segment but after a delay The case for a different action (i.e. load after drop) without ever having read the status of the previous action can happen during coordinator restarts or leader re-elections. Although, in these cases, it is not likely that the drop and load would happen concurrently. #### Different actions on same segment happening concurrently This is not expected to happen under normal conditions, and can only be caused by the Coordinator being in some weird state or a bug in the code. -- 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]
