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

fix: Move direct path misconfiguration log to before creating the first channel #2430

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

blakeli0
Copy link
Collaborator

@blakeli0 blakeli0 commented Jan 29, 2024

Fixes #2423
The root cause of the issue is that logDirectPathMisconfig() is called in the builder of InstantiatingGrpcChannelProvider, which could be called multiple times before it is fully instantiated. We should only call logDirectPathMisconfig() right before createChannel() which creates the first channel.

We can not move it to before createSingleChannel() because it is used for resizing channel regularly after a client is initialized, and we only want to log direct path misconfiguration once.

@blakeli0 blakeli0 requested a review from a team as a code owner January 29, 2024 16:37
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jan 29, 2024
@lqiu96
Copy link
Contributor

lqiu96 commented Jan 30, 2024

Thoughts on adding test which creates a InstantiatingGrpcChannelProvider, changes a property in the builder with it still misconfigured, and recreates it? Essentially runs InstantiatingGrpcChannelProvider.{newBuilder|toBuilder()}.build() twice. The FakeLogHandler should only show the misconfiguration log once since the logging should occur prior to channel creation and on .build().

Might be beneficial so we don't accidentally refactor logDirectPathMisconfig() into the channel creation logic in the future.

@blakeli0
Copy link
Collaborator Author

Thoughts on adding test which creates a InstantiatingGrpcChannelProvider, changes a property in the builder with it still misconfigured, and recreates it? Essentially runs InstantiatingGrpcChannelProvider.{newBuilder|toBuilder()}.build() twice. The FakeLogHandler should only show the misconfiguration log once since the logging should occur prior to channel creation and on .build().

Thanks! Added a test case that calling builder should not log anything.

@blakeli0 blakeli0 added the automerge Merge the pull request once unit tests and other checks pass. label Jan 31, 2024
Copy link

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@gcf-merge-on-green gcf-merge-on-green bot merged commit 9916540 into main Jan 31, 2024
37 of 39 checks passed
@gcf-merge-on-green gcf-merge-on-green bot deleted the fix-directpath-log branch January 31, 2024 05:10
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DirectPath Misconfiguration emitted for incorrect Credentials but DirectPath still works
3 participants