gortiz commented on code in PR #15071:
URL: https://github.com/apache/pinot/pull/15071#discussion_r1963612826
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LeafStageTransferableBlockOperator.java:
##########
@@ -182,6 +183,26 @@ protected TransferableBlock getNextBlock()
}
}
+ @Override
+ protected void earlyTerminate() {
+ super.earlyTerminate();
+ cancelSseTasks();
+ }
+
+ @Override
+ public void cancel(Throwable e) {
+ super.cancel(e);
+ cancelSseTasks();
+ }
+
+ @VisibleForTesting
+ protected void cancelSseTasks() {
+ Future<Void> executionFuture = _executionFuture;
Review Comment:
> _executionFuture is only set once right?
Yes, it is. But if someone changes the code tomorrow and `_executionFuture`
can be set to null or other values, then we don't want to be cancelling another
future or throwing a NPE. I'm a bit paranoid here, but there is almost no
reason to write the unsafe code:
1. The static analyzers may complain about possible NPE (at least my IDE was
complaining)
2. The bytecode and probably the JITed code will be faster, as we won't need
to read a volatile attribute twice.
The cost is a slightly more complex code. I think it is worth it
--
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]
