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

DefaultDgsFederationResolver now handles failed completion stages ret… #730

Conversation

victorlevasseur
Copy link
Contributor

@victorlevasseur victorlevasseur commented Nov 10, 2021

…urned from entity fetchers properly

Pull request checklist

  • Please read our contributor guide
  • Consider creating a discussion on the discussion forum
    first
  • Make sure the PR doesn't introduce backward compatibility issues
  • Make sure to have sufficient test cases

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #726

It allows the DefaultDgsFederationResolver to properly handle exceptions thrown by failed completable futures.

I used the dataloader Try object to ease the handling of exception and to make sure to have a single path of execution for exceptions thrown in the entity fetcher body and also the exceptions thrown by failed completable futures. That's why the whole call to each entity fetchers are wrapped in Try.tryCallable (to handle exceptions thrown directly in the entity fetcher) and then unwrapped when combined with the result of the CompletableFuture (to handle exceptions thrown by a failed completable future).

I added a new tests to check that this use case now works.

@srinivasankavitha
Copy link
Contributor

Thanks for the PR @victorlevasseur. Will test this out and get back to you shortly.

Copy link
Contributor

@berngp berngp left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks for doing this.

@berngp berngp merged commit 01240a4 into Netflix:master Nov 11, 2021
@victorlevasseur victorlevasseur deleted the bugfix/entity-fetcher-failed-completion-stage branch November 11, 2021 18:36
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.

None yet

3 participants