-
Notifications
You must be signed in to change notification settings - Fork 123
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
chore: add directpath_enabled attribute #3380
Conversation
29cbe93
to
2bf89b8
Compare
Warning: This pull request is touching the following templated files:
|
2bf89b8
to
340d9e9
Compare
@@ -247,6 +247,11 @@ public static final GrpcSpannerStub create(ClientContext clientContext) throws I | |||
return new GrpcSpannerStub(SpannerStubSettings.newBuilder().build(), clientContext); | |||
} | |||
|
|||
public static final GrpcSpannerStub create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a generated file, so you can't manually update it. (That is: you can update it, but the next round of code generation will overwrite your changes)
/* isAdminClient = */ false, isEmulatorEnabled(options, emulatorHost))) | ||
.build(); | ||
ClientContext clientContext = ClientContext.create(spannerStubSettings); | ||
this.spannerStub = GrpcSpannerStub.create(spannerStubSettings, clientContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below: This calls a method in a generated file, and this method is manually added to that generated file. That will be overwritten the next time the code generation is run, so this won't work.
.build(); | ||
ClientContext clientContext = ClientContext.create(spannerStubSettings); | ||
this.spannerStub = GrpcSpannerStub.create(spannerStubSettings, clientContext); | ||
BuiltInMetricsConstant.DIRECT_PATH_ENABLED = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user could create multiple Spanner instances. Then this code will be invoked multiple times. There is no guarantee that isDirectPath()
will return the same value each time, as it depends on (AFAIK) the credentials that are used. DIRECT_PATH_ENABLED
is a public static variable that should preferably only be set once (or changed to an instance field of the SpannerImpl
class or one of the other underlying classes)
As part of this bug , modification were made in ClientContext which can be used to fetch
isDirectPath()
attribute.Recommendation was to pass clientContext in the
GrpcSpannerStub.create
instead of SpannerStubSetting.It was working for the regular calls, however, all the test cases using timeouts were failing.
Ideally this should have worked the similar way. However , the code here is not using the spannerStubSettings from clientContext, instead creating a new
SpannerStubSettings.newBuilder().build()
Added a new method in the
GrpcSpannerStub
file which accepts both stubSettings and clientContext.