echonesis commented on code in PR #11294:
URL: https://github.com/apache/ozone/pull/11294#discussion_r4069087498
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientShortCircuit.java:
##########
@@ -373,14 +344,7 @@ void sendRequest(RequestEntry entry) {
ContainerCommandRequestProto request = entry.getRequest();
try {
final RequestKey key = new RequestKey(request.getClientId(),
request.getCallId());
- TimerTask task = new TimerTask() {
- @Override
- public void run() {
- requestTimeout(key);
- }
- };
- entry.setTimerTask(task);
- timer.schedule(task, readTimeoutMs);
+ scheduler.schedule(key, entry, readTimeoutMs);
sentRequests.put(key, entry);
Review Comment:
Just a question: the timeout task is scheduled before the entry is added to
`sentRequests`. In the unlikely event that the task fires before the following
put, requestTimeout would find no entry, and the request would then have no
active timeout. Would it be slightly safer to publish the entry before
scheduling the task?
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientShortCircuit.java:
##########
@@ -653,4 +591,45 @@ public boolean equals(Object obj) {
&& this.blockLocalId == that.blockLocalId;
}
}
+
+ class TimeoutScheduler {
+ private Timer timer;
+ private int cancelCount = 0;
+
+ synchronized void init(String prefix) {
+ Preconditions.assertNull(timer, "timer");
+ timer = new Timer(prefix + "-Timer");
+ }
+
+ synchronized void schedule(RequestKey key, RequestEntry entry, int
timeoutMs) {
+ if (timer == null) {
+ return;
+ }
+ final TimerTask task = new TimerTask() {
+ @Override
+ public void run() {
+ requestTimeout(key);
+ }
+ };
+ timer.schedule(task, timeoutMs);
+ entry.getFuture().whenComplete((r, e) -> cancel(task));
Review Comment:
Cancelling the timeout task on every future completion changes the existing
send-failure behavior.
When sendRequest completes the future exceptionally after an IOException,
this callback cancels the task that would otherwise remove the entry from
sentRequests, leaving the failed request retained indefinitely.
Could this completion callback also remove the exact (key, entry), or could
the send-failure path remove it before completing the future?
--
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]
