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

rls:Fix throttling in route lookup (b/262779100) #9874

Merged
merged 12 commits into from
Feb 6, 2023

Conversation

larry-safran
Copy link
Contributor

@larry-safran larry-safran commented Feb 3, 2023

Fixes b/262779100

Previously, registerBackendResponse was being passed the opposite value from what it should have been. Thus, successes were being registered as throttled and all errors were registered as not throttled by the backend. Now it is only registered as throttled when there is an error and the cause is a ThrottledException.

@larry-safran larry-safran requested a review from temawi February 3, 2023 18:56
@larry-safran larry-safran changed the title rls:Fix throttling in route lookup rls:Fix throttling in route lookup (b/262779100) Feb 3, 2023
@larry-safran larry-safran added TODO:backport PR needs to be backported. Removed after backport complete TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved labels Feb 3, 2023
@larry-safran larry-safran added this to the 1.53 milestone Feb 3, 2023
Copy link
Contributor

@temawi temawi left a comment

Choose a reason for hiding this comment

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

Some test code would also be good. This change might be hard to test in a pure unit test, but if we had a test that used CachingRlsLbclient together with AdaptiveThrottler, this issue would have been caught long time ago.

rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java Outdated Show resolved Hide resolved
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I think it's clear we really need a test here. Did that turn out to not be readily feasible (especially given the soon release date)? (Edit: doh, I missed Terry's comment)

rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java Outdated Show resolved Hide resolved
Update test get_updatesLbState to check that the right values were registered for the throttler.
Update test get_updatesLbState to check that the right values were registered for the throttler.
Copy link
Contributor

@temawi temawi left a comment

Choose a reason for hiding this comment

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

Thanks Larry - looks to me these new tests would have caught the problem.

assertThat(adaptiveThrottler.throttledStat.get(time)).isEqualTo(1L);
}

private PickSubchannelArgsImpl getInvalidArgs(Metadata headers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor style nit: Methods starting with get usually return something that already exists. I think a prefix of new (or create, etc.) is better suited for methods that create new objects.

@larry-safran larry-safran merged commit 5983be1 into grpc:master Feb 6, 2023
@larry-safran larry-safran deleted the b262779100 branch February 6, 2023 23:19
larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 7, 2023
* Correct value being passed to throttler which had been backwards.

* Fix flaky test.

* Add a test using AdaptiveThrottler with a CachingRlsLBClient.

* Address test flakiness.
larry-safran added a commit that referenced this pull request Feb 7, 2023
* Correct value being passed to throttler which had been backwards.

* Fix flaky test.

* Add a test using AdaptiveThrottler with a CachingRlsLBClient.

* Address test flakiness.
@YifeiZhuang YifeiZhuang removed TODO:backport PR needs to be backported. Removed after backport complete TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved labels Feb 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants