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.

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

Jump to bottom

xds:Update logic to match A57 #9745

Merged
merged 5 commits into from
Dec 15, 2022
Merged

xds:Update logic to match A57 #9745

merged 5 commits into from
Dec 15, 2022

Conversation

larry-safran
Copy link
Contributor

@larry-safran larry-safran commented Dec 9, 2022 •
edited
Loading

Update logic to match A57-xds-client-failure-mode-behavior.md

  • When the ads stream is closed only send errors to subscribers that haven't yet gotten results to match spec
  • Change timer creation logic to wait until the adsStream is ready before creating the timer to mark resources absent

@larry-safran
xds:Change timer creation logic to wait until the adsStream is ready …
8d41a43 
…before creating the timer to mark resources absent.
@larry-safran
xds:When the ads stream is closed only send errors to subscribers tha…
9b997c6 
…t haven't yet gotten results to match spec.
@larry-safran
Use a blocking queue to avoid the 2-second sleep.
ac841be 
For some inexplicable reason the following call.verifyRequest fails only for the V2 test and only from command line not IDE unless there is some Thread.sleep, even if it is only 1-millis.
@larry-safran
Copy link
Contributor Author

@sanjaypujare Can you please re-review and approve?

@sanjaypujare
Copy link
Contributor

@sanjaypujare Can you please re-review and approve?

Okay, will get to this once I am done with my current task

@sanjaypujare
Copy link
Contributor

Added a few comments. Also I didn't find a test for "When the ads stream is closed only send errors to subscribers that haven't yet gotten results". It will be good to add a test for that too.

sanjaypujare
sanjaypujare reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

Made a few comments, once addressed will re-review

@sergiitk sergiitk removed their request for review December 14, 2022 20:23
@larry-safran
Copy link
Contributor Author

Comments addressed. Note that the feature you refer to is addressed by existing tests at 3211 and 3318.

sanjaypujare
sanjaypujare approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LGTM

@larry-safran larry-safran enabled auto-merge (squash) December 15, 2022 21:54
@larry-safran larry-safran added kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary and removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Dec 15, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 15, 2022
@larry-safran larry-safran merged commit 46ed02e into grpc:master Dec 15, 2022
@larry-safran larry-safran deleted the xdsClientFailureMode branch December 16, 2022 01:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sanjaypujare sanjaypujare sanjaypujare approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@larry-safran @sanjaypujare @grpc-kokoro