Update ListenableFutureUtil.java with log message #352
Conversation
| @@ -47,7 +47,7 @@ public boolean cancel(Exception rootReason) { | |||
| if (shouldCancelTask && !future.isCancelled()) { | |||
| boolean futureCancelResult = future.cancel(true); | |||
| if (!futureCancelResult) { | |||
| LOGGER.warn("Unexpected: GRPC future was not cancelled but new attempt to cancel also failed."); | |||
| LOGGER.warn("Unexpected: Task {} future was not cancelled but new attempt to cancel also failed.", future.getClass.getName()); | |||
There was a problem hiding this comment.
I am not clear about this change. ListenableFuture is from gRPC client stub invocation, that is why previous warning mentioned GRPC future, it is actually not a ParSeq task? What extra value future.getClass.getName() will be provided here? Most cases it will be ListenableFuture?
There was a problem hiding this comment.
So my understanding truly from the naming convention of the function is
-
The class is ListenableFutureUtil is providing utility methods and the implementations or contract are quite generic . the parameters is of Type ListenableFuture hence the errors or warnings should be quite generic .
Giving warning unexpected Grpc future was not cancelled is more concrete
Rather if the fromListenableFuture function had parameters GrpcFuture this message would have made sense to me -
By removing GRPC I just wanted to give more generic message fetching from the task details
I guess you're right , future.getClass.getName() might not be useful here, hence should I replace with task.name() ? which is constructed in above lines
There was a problem hiding this comment.
task.name() will not be accurate either. This is really talking about the incoming ListenableFuture is not cancelled properly, nothing about Parseq task.
This is a bug fix .
The error message seems a little dubious
The function is accepting task of Type Future but logs message with the task of type GRPC