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

propagate errors during async refresh #637

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

Conversation

viqtor
Copy link
Contributor

@viqtor viqtor commented Aug 20, 2023

resolves #636

@viqtor viqtor force-pushed the consumer-refresh-errors branch from d9dad86 to 0ea3c95 Compare August 21, 2023 06:33
@Sunjeet
Copy link
Contributor

Sunjeet commented Sep 8, 2023

Thanks for this contribution. Changing the return type of the triggerAsyncRefresh method from void to CompletableFuture<Void> would cause binary backwards incompatibility. Is there a reason to not simply use the synchronous flavor triggerRefresh/triggerRefreshTo in your application?

@Sunjeet Sunjeet self-requested a review September 8, 2023 19:59
@viqtor
Copy link
Contributor Author

viqtor commented Sep 9, 2023

Is there a reason to not simply use the synchronous flavor triggerRefresh/triggerRefreshTo in your application?

We have been following the reference implementation. We have since switched to the sync alternative since dicsovering this issue but it may lead to false assumptions when you pass in a refreshExectutor to your consumer that isn't used.

I just noticed that the same method is also used during the getInitialLoad() which wont trip the catch block.

Would an alternative solution be to deprecate these methods and add new ones with the new return type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

errors during consumer async refresh are hidden
2 participants