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

internal/grpc: move filtering of Values to Register #426

Closed
davecheney opened this issue Jun 5, 2018 · 7 comments
Closed

internal/grpc: move filtering of Values to Register #426

davecheney opened this issue Jun 5, 2018 · 7 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@davecheney
Copy link
Contributor

Currently the grpc package and xds handler can filter results returned by cache.Values by a filter constructed from the DiscoveryRequest. However the cache.Register method lacks a way of filtering notifications and thus registers for all changes to the cache. This mean when the channel passed to Register fires, the update may not match the filter supplied and the result is a noop.

This is better than #350, but has lead to at least two issues, #363 and #420, and possibly others.
@davecheney davecheney self-assigned this Jun 18, 2019
@davecheney davecheney added this to the 0.15.0 milestone Jun 18, 2019
@lrouquette lrouquette mentioned this issue Jun 18, 2019
Closed
@phylake
Copy link

phylake commented Jun 20, 2019

Is this the cause of the N^2 behavior we're seeing on EDS updates? For every EDS DiscoveryRequest we're pretty sure the entire EDS config is being sent back

@phylake
Copy link

phylake commented Jul 10, 2019

EDS is the only xDS that will benefit from this right? It feels like #1166 and #1172 but with a bit different description. Also, I'd still appreciate a response to my previous comment.

@stevesloka
Copy link
Member

@phylake solving #1166 & #1172 I think will drastically reduce the number of updates overall on a busy cluster. I'm not personally 100% certain the answer to the n^2 behavior you are referencing, so I don't want to give a guess for a response.

@davecheney davecheney modified the milestones: 0.15.0, 1.0.0-beta.1 Aug 23, 2019
@davecheney davecheney added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 10, 2019
davecheney added a commit to davecheney/contour that referenced this issue Sep 10, 2019
@davecheney
internal/contour: consolidate Register/Notify on Cond type
c9bb790 
Updates projectcontour#426

Signed-off-by: Dave Cheney <dave@cheney.net>
@davecheney davecheney mentioned this issue Sep 10, 2019
Merged
davecheney added a commit that referenced this issue Sep 11, 2019
@davecheney
internal/contour: consolidate Register/Notify on Cond type
e0ef20c 
Updates #426

Signed-off-by: Dave Cheney <dave@cheney.net>
@davecheney
Copy link
Contributor Author

EDS updates are now filtered by the resource hint that envoy provides in its discovery request.

@davecheney davecheney removed their assignment Sep 12, 2019
@davecheney davecheney modified the milestones: 1.0.0-beta.1, Backlog Sep 12, 2019
@davecheney
Copy link
Contributor Author

Nothing more is planned for this issue before 1.0. Moving to the backlog

@youngnick youngnick self-assigned this Mar 8, 2021
@skriss skriss removed this from the Backlog milestone Jul 25, 2022
@youngnick youngnick removed their assignment Aug 4, 2022
@sunjayBhatia sunjayBhatia added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 26, 2022
@github-actions GitHub Actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

  • Mark this Issue as fresh by commenting
  • Close this Issue
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2024
@github-actions GitHub Actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

  • Mark this Issue as fresh by commenting
  • Close this Issue
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@davecheney @phylake @stevesloka @skriss @sunjayBhatia @youngnick