Skip to content

Update ListenableFutureUtil.java with log message #352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update ListenableFutureUtil.java with log message #352

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ts/parseq-guava-interop/src/main/java/com/linkedin/parseq/guava/ListenableFutureUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@ayush571995 ayush571995 Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my understanding truly from the naming convention of the function is

  1. 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

  2. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

task.name() will not be accurate either. This is really talking about the incoming ListenableFuture is not cancelled properly, nothing about Parseq task.

}
}
return shouldCancelTask;
Expand Down