Skip to content
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.

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

ISPN-13798 Make TxCompletionNotificationCommand non blocking #10009

Merged
merged 1 commit into from Apr 5, 2022

Conversation

pruivo
Copy link
Member

@pruivo pruivo commented Mar 30, 2022

}
return Collections.emptyMap();
return CompletableFutures.completedEmptyMap();
Copy link
Member

Choose a reason for hiding this comment

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

Looks like something is missing, this only ever returns a completed empty map.

Copy link
Member Author

Choose a reason for hiding this comment

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

The TxCompletionNotificationCommands is async so, there is nothing to return and this method is no longer used by any other command.
I was in doubt if I should

  • return void
  • remove the method from StateTransferManager since it is only used by TxCompletionNotificationCommands

Copy link
Member

Choose a reason for hiding this comment

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

When I see this method I expect the returned Stage to complete when all processing related to this method is done. And if I want I can ignore the stage and let it process in the background. If the user can't do this, we should change it to void and document that stuff processes asynchronously in the background.

Also if the only place that uses this is the command and it doesn't require state from the StateTransferManger lets move it to the command directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deprecated the method from StateTransferManager. 👍

@wburns wburns merged commit e07423a into infinispan:main Apr 5, 2022
@wburns
Copy link
Member

wburns commented Apr 5, 2022

Integrated into main, thanks @pruivo !

@pruivo pruivo deleted the t_13798 branch April 6, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants