Skip to content

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

Merged
gcf-merge-on-green[bot] merged 5 commits intomainfrom
fix-directpath-log
Jan 31, 2024
Merged

fix: Move direct path misconfiguration log to before creating the first channel#2430
gcf-merge-on-green[bot] merged 5 commits intomainfrom
fix-directpath-log

Conversation

@blakeli0
Copy link
Copy Markdown
Contributor

@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 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
Copy Markdown
Member

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
Copy Markdown
Contributor 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
@sonarqubecloud
Copy link
Copy Markdown

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

@sonarqubecloud
Copy link
Copy Markdown

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
@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