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

[#583][Delta] Ensure resources are properly sent again if envoy unsubscribes then subscribes again to a resource #586

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

valerian-roche
Copy link
Contributor

This PR is part of the fix for #583 related to resources being unsubscribed from then subscribed to again
This one applies to both simple and linear cache when using the delta protocol.
Actionable cases would likely only impact delta-ADS as EDS outside of ADS does not share streams and will therefore usually not have multiple resources this way

The logger issue is related to sotw-ADS which is part of a followup PR
This PR is built on top of #584 as the sotw part will be heavily relying on it

…e uniform between sotw and delta

Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
…nvoy unsubscribes then subscribes again to a resource

Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@haoruolei
Copy link

is this work still active?

@valerian-roche
Copy link
Contributor Author

This work builds on #584, which itself now depends on rolling back #615. Sadly graphite and branch stacking cannot be done across forks
Alec has had few cycles recently to review, but when he becomes available I hope to have those move ahead

@alecholmez
Copy link
Contributor

@valerian-roche I just took a quick pass at the PR. I like the changes. We'll have to be careful with some of the breaking API changes and call that out when we release. Or we keep them around and tag them as // DEPRECATED in a comment and pull them out in the next release to at least give folks sometime to move away. What do you think about that?

Can you rebase? I'm cool with shipping this through if you're ready to pull it out of draft.

@valerian-roche
Copy link
Contributor Author

@alecholmez On our side we pushed for multiple fixes in order to support grpc-xds natively on this fork. We have been running for a while with this (also got some fix on grpc to have it properly working).
It does include the API breaking changes in this PR. I was hoping to get #714 merged first to be able to break API here, as imo if we remove the protobuf dep we can be a more pushy on those changes. Wdyt?

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

Successfully merging this pull request may close these issues.

3 participants