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

`DefaultServiceDiscoveryRetryStrategy` may emit duplicated SD events #1586

Merged

idelpivnitskiy merged 8 commits into apple:main from idelpivnitskiy:DefaultServiceDiscoveryRetryStrategy

May 28, 2021

Member

idelpivnitskiy commented May 27, 2021 •

edited

Loading

Motivation:

When DefaultServiceDiscoveryRetryStrategy retains addresses till
success and recovers from the previous SD error, it doesn't check which
of new addresses were previously retained. As the result, it may
propagate duplicated "available" events to RoundRobinLoadBalancer.
RRLB does not support duplicated entries. It may lead to retaining
duplicated addresses after they become unavailable, causing failures
of requests.

Modifications:

Filter out previously retained events when we recover from SD-error;
Do not return duplicated unavailable events on subsequent SD-errors;
Enhance tests to catch these use-cases;

Result:

DefaultServiceDiscoveryRetryStrategyTest does not emit duplicated SD
events for the same availability state => RRLB does not retain duplicated
entries.


          DefaultServiceDiscoveryRetryStrategy may emit duplicated SD events

3bb162e

Motivation:

When `DefaultServiceDiscoveryRetryStrategy` retains addresses till
success and recovers from the previous SD error, it doesn't check which
of new addresses were previously retained. As the result, it may
propagate duplicated "available" events to `RoundRobinLoadBalancer`.

Modifications:

- Filter out previously retained events when we recover from SD-error;
- Enhance `overlapAddressPostRetry()` tests to catch this use-case;
- Filter out events for duplicated addresses on each resolution;
- Add `duplicatedAddresses()` test to catch this use-case;

Result:

`DefaultServiceDiscoveryRetryStrategyTest` does not emit duplicated SD
events for the same availability state.

idelpivnitskiy requested review from bondolo and Scottmitch

May 27, 2021 22:17

Scottmitch reviewed

View reviewed changes

...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java Outdated Show resolved Hide resolved

...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java Outdated

		}
		// FIXME: remove addresses that are available and unavailable in the same events collection

Member

Scottmitch May 27, 2021

I think "last event wins" is the expected strategy. what did you have in mind here?

Member Author

idelpivnitskiy May 27, 2021 •

edited

Loading

If Collection has two entries:

foo, available
foo, unavailable

we will propagate both events to RRLB instead of propagating nothing. It will add more work for RRLB to add the address and remove it immediately. Due to concurrency between threads, that address can be used by a client before the 2nd event is processed

...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java Outdated Show resolved Hide resolved

...http-api/src/test/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategyTest.java Outdated Show resolved Hide resolved

...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java Outdated Show resolved Hide resolved

bondolo reviewed

View reviewed changes

Contributor

bondolo left a comment

not confident I understand it will enough to approve or disapprove.

...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java Show resolved Hide resolved

idelpivnitskiy added 2 commits

May 27, 2021 16:47


          Avoid duplicated unavailable events on subsequent errors


          Apply patch

idelpivnitskiy requested a review from Scottmitch

May 28, 2021 00:30

idelpivnitskiy added 3 commits

May 27, 2021 18:00


          Revert a patch and add a new test

f920ad5


          Fix the test

d5795b1


          Use || instead of bitwise

9946bf7

Scottmitch approved these changes

View reviewed changes

Member

Scottmitch left a comment

good catch @idelpivnitskiy !

...http-api/src/test/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategyTest.java Outdated Show resolved Hide resolved

idelpivnitskiy added 2 commits

May 27, 2021 18:20


          use tmp variable for addr1

d77d37d


          Revert unnecessary changes for tests

d9ccb0f

idelpivnitskiy merged commit a51d2f2 into apple:main

idelpivnitskiy deleted the DefaultServiceDiscoveryRetryStrategy branch

May 28, 2021 03:12

idelpivnitskiy added a commit that referenced this pull request


          DefaultServiceDiscoveryRetryStrategy may emit duplicated SD events (#…

41983c2

…1586)

Motivation:

When `DefaultServiceDiscoveryRetryStrategy` retains addresses till
success and recovers from the previous SD error, it doesn't check which
of new addresses were previously retained. As the result, it may
propagate duplicated "available" events to `RoundRobinLoadBalancer`.
RRLB does not support duplicated entries. It may lead to retaining
duplicated addresses after they become unavailable, causing failures
of requests.

Modifications:

- Filter out previously retained events when we recover from SD-error;
- Do not return duplicated `unavailable` events on subsequent SD-errors;
- Enhance tests to catch these use-cases;

Result:

`DefaultServiceDiscoveryRetryStrategyTest` does not emit duplicated SD
events for the same availability state => RRLB does not retain duplicated
entries.

idelpivnitskiy added a commit that referenced this pull request


          DefaultServiceDiscoveryRetryStrategy may emit duplicated SD events (#…

5556e22

…1586)

Motivation:

When `DefaultServiceDiscoveryRetryStrategy` retains addresses till
success and recovers from the previous SD error, it doesn't check which
of new addresses were previously retained. As the result, it may
propagate duplicated "available" events to `RoundRobinLoadBalancer`.
RRLB does not support duplicated entries. It may lead to retaining
duplicated addresses after they become unavailable, causing failures
of requests.

Modifications:

- Filter out previously retained events when we recover from SD-error;
- Do not return duplicated `unavailable` events on subsequent SD-errors;
- Enhance tests to catch these use-cases;

Result:

`DefaultServiceDiscoveryRetryStrategyTest` does not emit duplicated SD
events for the same availability state => RRLB does not retain duplicated
entries.

idelpivnitskiy added a commit that referenced this pull request


          DefaultServiceDiscoveryRetryStrategy may emit duplicated SD events (#…

eac9813

…1586)

Motivation:

When `DefaultServiceDiscoveryRetryStrategy` retains addresses till
success and recovers from the previous SD error, it doesn't check which
of new addresses were previously retained. As the result, it may
propagate duplicated "available" events to `RoundRobinLoadBalancer`.
RRLB does not support duplicated entries. It may lead to retaining
duplicated addresses after they become unavailable, causing failures
of requests.

Modifications:

- Filter out previously retained events when we recover from SD-error;
- Do not return duplicated `unavailable` events on subsequent SD-errors;
- Enhance tests to catch these use-cases;

Result:

`DefaultServiceDiscoveryRetryStrategyTest` does not emit duplicated SD
events for the same availability state => RRLB does not retain duplicated
entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet