1996fanrui commented on code in PR #26453:
URL: https://github.com/apache/flink/pull/26453#discussion_r2043374262
##########
flink-runtime/src/main/java/org/apache/flink/runtime/operators/coordination/SubtaskGatewayImpl.java:
##########
@@ -111,6 +111,10 @@ public CompletableFuture<Acknowledge>
sendEvent(OperatorEvent evt) {
sendResult.whenCompleteAsync(
(success, failure) -> {
if (failure != null &&
subtaskAccess.isStillRunning()) {
+ if (failure instanceof TaskNotRunningException
+ && evt.isOptional()) {
Review Comment:
From the code implementation, we assume the event will be re-sent later
after being lost. Is it possible to rename the `isOptional` to
`isRedeliverable` or `isRecurringWithRetry`? Or something else?
The `isOptional` is a bit general, I'm afraid one event without re-send
mechanism, but dev think it's optional in future development. It may lead to
potential bugs due to misunderstanding.
WDYT?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/operators/coordination/SubtaskGatewayImpl.java:
##########
@@ -111,6 +111,10 @@ public CompletableFuture<Acknowledge>
sendEvent(OperatorEvent evt) {
sendResult.whenCompleteAsync(
(success, failure) -> {
if (failure != null &&
subtaskAccess.isStillRunning()) {
+ if (failure instanceof TaskNotRunningException
Review Comment:
Do we need to use `ExceptionUtils.findThrowable(failure ,
TaskNotRunningException.class)` here to cover more cases?
TaskNotRunningException may be wrapped by other exceptions.
Also, it's better to improve the `optionalEventsIgnoreTaskNotRunning` test
as well if this comment is reasonable.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
