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

delta cds: add on-demand cds support. #8984

Closed
wants to merge 3 commits into from

Conversation

aaron-ai
Copy link
Member

@aaron-ai aaron-ai commented Nov 12, 2019

Description: this PR allows user can start a spontaneous DeltaDiscoveryRequests for cds (AKA on-demand cds) by using addToClusterInterest or updateClusterInterest in cluster_manager.
Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]: #8948

@aaron-ai aaron-ai force-pushed the develop branch 3 times, most recently from 6186a92 to a5a0867 Compare November 12, 2019 07:06
@htuch htuch requested review from lambdai and fredlas November 12, 2019 16:20
@htuch
Copy link
Member

htuch commented Nov 12, 2019

@lambdai could you do a first pass on this (and @fredlas if you have cycles)? Thanks

Copy link
Contributor

@fredlas fredlas left a comment

Choose a reason for hiding this comment

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

As a heads up, the actual clean version of the gRPC xDS client code, which will give you a nice firm footing to build off of, is what is being un-reverted by #8974. Fortunately, the merge conflicts will be 100% mechanical: the actual interesting logic you're building on in the delta part is already in place. There will just be a little file renaming and whatnot.

@@ -39,6 +39,12 @@ void DeltaSubscriptionImpl::updateResourceInterest(
stats_.update_attempt_.inc();
}

void DeltaSubscriptionImpl::addResourceInterest(const std::set<std::string>& add_these_names) {
watch_ = context_->addOrUpdateWatch(type_url_, watch_, add_these_names, *this,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think GrpcMux::addOrUpdateWatch does what you are needing it to do. You are trying to use it as "add TO the existing watch", but what it's actually going to do is just overwrite with add_these_names. The "add" part of "addOrUpdateWatch" means "add a watch to the map if there was none" (i.e. the watch argument is nullptr).

So, looks like this function was named entirely from the point of view of someone (i.e. me) implementing GrpcMux and WatchMap, rather than what would make sense for an external user. My apologies! Maybe it should just be called updateWatch.

Anyways, I think what you need to do is add a function to WatchMap. I think what you want is to give WatchMap::updateWatchInterest a sibling, maybe WatchMap::addToWatchInterest, that basically does the exact same as what updateWatchInterest does, except newly_added_to_watch is just the input argument, and newly_removed_from_watch is the empty set.

@aaron-ai
Copy link
Member Author

As a heads up, the actual clean version of the gRPC xDS client code, which will give you a nice firm footing to build off of, is what is being un-reverted by #8974. Fortunately, the merge conflicts will be 100% mechanical: the actual interesting logic you're building on in the delta part is already in place. There will just be a little file renaming and whatnot.

Thanks for reply !! I will fix it.

@aaron-ai aaron-ai force-pushed the develop branch 3 times, most recently from d8f227e to 6e8cccb Compare November 13, 2019 07:42
@aaron-ai aaron-ai requested a review from fredlas November 13, 2019 10:43
Signed-off-by: aaron-ai <yangkun.ayk@alibaba-inc.com>
Signed-off-by: aaron-ai <yangkun.ayk@alibaba-inc.com>
Signed-off-by: aaron-ai <yangkun.ayk@alibaba-inc.com>
@lambdai
Copy link
Contributor

lambdai commented Nov 15, 2019

I think the CdsApi require the watch interface but Iam not sure if I understand your full picture.

Your goal is to reduce the interested cluster, but the existing CdsApi is implicitly subscribe ALL clusters.

The code is vague if you want to use

  1. one CdsApi instance
  2. 1 for ALL_cluster and 1 CdsApi interested clusters.

I think above is the first step before we dive deeper

@aaron-ai
Copy link
Member Author

aaron-ai commented Nov 15, 2019

I think the CdsApi require the watch interface but Iam not sure if I understand your full picture.

Your goal is to reduce the interested cluster, but the existing CdsApi is implicitly subscribe ALL clusters.

The code is vague if you want to use

  1. one CdsApi instance
  2. 1 for ALL_cluster and 1 CdsApi interested clusters.

I think above is the first step before we dive deeper

Thanks, I just want to use one CdsApi instance for our need. and when I use DELTA_GRPC, our GRPC discovery service will only return the CDS resource we subscribe spontaneously.

@stale
Copy link

stale bot commented Nov 22, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2019
@stale
Copy link

stale bot commented Nov 29, 2019

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants