Skip to content

CompositeChannelCredentials: Comparator implementation#28902

Merged
yashykt merged 2 commits intogrpc:masterfrom
yashykt:CompositeCredentialsCmp
Feb 17, 2022
Merged

CompositeChannelCredentials: Comparator implementation#28902
yashykt merged 2 commits intogrpc:masterfrom
yashykt:CompositeCredentialsCmp

Conversation

@yashykt
Copy link
Copy Markdown
Member

@yashykt yashykt commented Feb 17, 2022 •
edited
Loading

Context - #28844
This PR allows channels configured with the same/similar/equal CompositeChannelCredentials objects to use the same underlying subchannels. To avoid sharing subchannels with other channels, the channel arg GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL can be set to true.
cc @markdroth

@yashykt yashykt requested a review from yihuazhang February 17, 2022 02:55
@yashykt yashykt added the release notes: no Indicates if PR should not be in release notes label Feb 17, 2022
yihuazhang
yihuazhang reviewed Feb 17, 2022
View reviewed changes
Comment thread test/core/security/credentials_test.cc
grpc_call_credentials_release(md_creds);
}

TEST(CredentialsTest, TestCompositeChannelCredsCompareSuccess) {
Copy link
Copy Markdown
Contributor

@yihuazhang yihuazhang Feb 17, 2022

Choose a reason for hiding this comment

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

Can we add another test case that tests composite call creds? Just to have a more complete test coverage?

Copy link
Copy Markdown
Member Author

@yashykt yashykt Feb 17, 2022

Choose a reason for hiding this comment

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

Changes for the CompositeCallCredentials comparator haven't been made yet though, so the test wouldn't be useful

Copy link
Copy Markdown
Contributor

@yihuazhang yihuazhang Feb 17, 2022

Choose a reason for hiding this comment

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

Ah got it.

@yashykt yashykt enabled auto-merge (squash) February 17, 2022 23:00
@yashykt yashykt merged commit 2fdb5f9 into grpc:master Feb 17, 2022
yashykt added a commit that referenced this pull request Feb 18, 2022
@yashykt
Revert "CompositeChannelCredentials: Comparator implementation (#28902)"
2da4298 
This reverts commit 2fdb5f9.
@yashykt yashykt mentioned this pull request Feb 18, 2022
Merged
yashykt added a commit that referenced this pull request Feb 18, 2022
@yashykt
Revert "CompositeChannelCredentials: Comparator implementation (#28902)…
e6a54ef 
…" (#28919)

This reverts commit 2fdb5f9.
yashykt added a commit to yashykt/grpc that referenced this pull request Feb 18, 2022
@yashykt
Revert "Revert "CompositeChannelCredentials: Comparator implementation (
8cf9df0 
grpc#28902)" (grpc#28919)"

This reverts commit e6a54ef.
@copybara-service copybara-service Bot added the imported Specifies if the PR has been imported to the internal repository label Feb 18, 2022
yashykt added a commit that referenced this pull request Feb 22, 2022
@yashykt
Revert "Revert "CompositeChannelCredentials: Comparator implementatio…
3f19e0c 
…n ( #28902)" (#28919)" (#28918)

* CompositeChannelCredentials: Comparator implementation retry

* Fix test
@yashykt yashykt added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Feb 23, 2022
@yashykt yashykt deleted the CompositeCredentialsCmp branch May 18, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yihuazhang yihuazhang yihuazhang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bloat/none imported Specifies if the PR has been imported to the internal repository lang/core perf-change/none release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yashykt @yihuazhang