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

Make DiscoveryServer stream ID global #157

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

Conversation

smtilden
Copy link

No description provided.

@smtilden smtilden force-pushed the stilden/global-stream-counter branch from 2dddc8f to e8ea8ba Compare December 16, 2020 20:45
@smtilden
Copy link
Author

When surfacing V2 and V3 streams alongside each other in envoy-control,
DiscoveryServerCallbacks are unable to differentiate between V2 & V3 ADS
upon onStreamClose(), onStreamCloseWithError().

This means that any DiscoveryServerCallback that keeps state cannot pivot
on stream IDs, since there will be duplicate between V2 & V3. This change
creates a global StreamCounter, which ensures stream IDs will be unique
across V2 & V3 streams.

When surfacing V2 and V3 streams alongside each other in envoy-control,
DiscoveryServerCallbacks are unable to differentiate between V2 & V3 ADS
upon onStreamClose(), onStreamCloseWithError().

This means that any DiscoveryServerCallback that keeps state cannot pivot
on stream IDs, since there will be duplicate between V2 & V3. This change
creates a global StreamCounter, which ensures stream IDs will be unique
across V2 & V3 streams.

Signed-off-by: Stephanie Tilden <stephanietilden@squareup.com>
@smtilden smtilden force-pushed the stilden/global-stream-counter branch from e8ea8ba to 91da9e9 Compare December 16, 2020 20:47
@codecov-io
Copy link

Codecov Report

Merging #157 (91da9e9) into master (ff834cb) will decrease coverage by 0.07%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #157      +/-   ##
============================================
- Coverage     88.22%   88.15%   -0.08%     
- Complexity      298      300       +2     
============================================
  Files            31       32       +1     
  Lines           977      979       +2     
  Branches         78       78              
============================================
+ Hits            862      863       +1     
- Misses           85       86       +1     
  Partials         30       30              
Impacted Files Coverage Δ Complexity Δ
.../envoyproxy/controlplane/server/StreamCounter.java 66.66% <66.66%> (ø) 2.00 <2.00> (?)
...nvoyproxy/controlplane/server/DiscoveryServer.java 100.00% <100.00%> (ø) 6.00 <3.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff834cb...91da9e9. Read the comment docs.

@@ -21,7 +20,6 @@
final ConfigWatcher configWatcher;
final ProtoResourcesSerializer protoResourcesSerializer;
private final ExecutorGroup executorGroup;
private final AtomicLong streamCount = new AtomicLong();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you instead just make this field static?

Base automatically changed from master to main January 16, 2021 21:52
Copy link
Contributor

@baojr baojr left a comment

Choose a reason for hiding this comment

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

We've been running this in production for a while without issue.

Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

@slonka LGTM, can we merge it?

@slonka
Copy link
Member

slonka commented Feb 21, 2021

I will look at it tomorrow, if you're in a hurry then go for it.

@slonka
Copy link
Member

slonka commented Mar 8, 2021

@jakubdyszkiewicz do you want to release it or should I?

@mpuncel
Copy link
Contributor

mpuncel commented Jan 3, 2023

@slonka looks like we should probably merge this? is your approval still good?

@slonka
Copy link
Member

slonka commented Jan 4, 2023

@mpuncel - yeah 👍 - I just did not release it because the release process was broken at the time. If it's fixed then go ahead.

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.

6 participants