-
Notifications
You must be signed in to change notification settings - Fork 6
feat: refactor timeout #622
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
base: main
Are you sure you want to change the base?
Conversation
karel-rehor
left a comment
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 looks good for passing timeout arguments to underlying APIs. Maybe I've missed something, but I don't see client properties like writeTimeout and queryTimeout
To synchronize changes with other libraries, in addition to sending a timeout value as a function argument, we also need to set a default writeTimeout and queryTimeout value when instantiating the client or its configuration. This especially needs to be used on the query side (https://grpc.io/docs/guides/deadlines/#deadlines-on-the-client and https://grpc.io/blog/deadlines/).
Having a client property that gets reused with every call will be much more convenient for users, than having to set the timeout with every call.
9fc395e to
42575c1
Compare
vlastahajek
left a comment
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.
Why flood the write and query API with the timeout parameter?
Is it really necessary to change the timeout for each call?
queryTimeout is already part of the options and is used correctly.
We could add just writeTimeout to properly differentiate values.
|
Maybe? We should be pretty sure about use-cases before changing API.
What other libraries have the timeout parameter in the insert and query-like functions? I've checked Mongo and Postgresql and haven't found that. |
I'm not talking about other libraries like Mongo or Posgrest, but our client libs like influxdb3-java, influxdb3-python, influxdb3-go,.... |
|
Neither of the other InfuxDB 3 client libraries has a timeout as a parameter of the |
Hi @vlastahajek |
|
@NguyenHoangSon96, @vlastahajek
When working on the python client I added |
karel-rehor
left a comment
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.
I have some problems with some of the deprecated fields. queryTimeout is already an important value passed to the GrpcTransport, and likely should not be removed. It is also self descriptive. The option timeout can also simply be made more specific by creating a writeTimeout field which fulfills the same function.
| ...writeOptions?.headers, | ||
| }, | ||
| gzipThreshold: writeOptionsOrDefault.gzipThreshold, | ||
| timeout: writeOptions?.timeout ?? this._options.timeout, |
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.
| timeout: writeOptions?.timeout ?? this._options.timeout, | |
| timeout: writeOptions?.timeout ?? this._options.writeTimeout, |
I don't see the new writeTimeout option anywhere used
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.
It's already set in file InfluxDBClient line 133.
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.
The line you mentioned in the InfluxDBClient doesn't use writeTimeout.
In fact, the line 145 here should be only:
timeout: writeOptions?.timeoutbecause sendOptions are optional.
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.
I mean, line 133 in the file "InfluxDBClient" already sets the timeout inside the options object
timeout: this._options.writeOptions?.timeout ?? this._options.timeout,
, so in the file "WriteApiImpl", line 145, I just need to check between timeout in the options object with the timeout passed directly to the doWrite function through the writeOptions object.
timeout: writeOptions?.timeout ?? this._options.timeout,
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.
Explained here
| this._transport = impl.writeTransport(this._options) | ||
| this._writeApi = new WriteApiImpl({ | ||
| transport: this._transport, | ||
| ...this._options, | ||
| timeout: this._options.writeOptions?.timeout ?? this._options.timeout, | ||
| }) |
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._transport = impl.writeTransport(this._options) | |
| this._writeApi = new WriteApiImpl({ | |
| transport: this._transport, | |
| ...this._options, | |
| timeout: this._options.writeOptions?.timeout ?? this._options.timeout, | |
| }) | |
| this._transport = impl.writeTransport({ | |
| ...this._options, | |
| writeTimeout: this._options.writeOptions?.timeout ?? this._options.writeTimeout | |
| }) | |
| this._writeApi = new WriteApiImpl({ | |
| transport: this._transport, | |
| ...this._options, | |
| }) |
passing an explicit timeout doesn't have an effect, because transport is pre-created and passed. Still, you used the deprecated timeout instead of writeTimeout
| ...headers, | ||
| ...sendOptions.headers, | ||
| }, | ||
| timeout: sendOptions.timeout ?? this._defaultOptions.timeout, |
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.
| timeout: sendOptions.timeout ?? this._defaultOptions.timeout, | |
| timeout: sendOptions.timeout ?? this._defaultOptions.writeTimeout, |
Utilize writeTimeout passed via WriteApi from InfluxDBClient
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.
I just thought that NodeHttpTransport should stay away from our "write" logic timeout, so it should only care about the 'timeout" but shouldn't care if it is "writeTimeout" or whatever "....Timeout".
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.
And in the file "InfluxDBClient" line 133, when initializing the NodeHttpTransport, I set the "timeout"
timeout: this._options.writeOptions?.timeout ?? this._options.timeout,
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.
I just thought that NodeHttpTransport should stay away from our "write" logic timeout, so it should only care about the
timeoutbut shouldn't care if it is "writeTimeout" or whatever "....Timeout".
NodeHttpTransport has ConnectionOptions as a constructor parameter, so it cannot ignore the deprecation of the timeout option and add a new writeTimeout option.
And in the file "InfluxDBClient" line 133, when initializing the NodeHttpTransport, I set the "timeout": this._options.writeOptions?.timeout ?? this._options.timeout,`
The timeout option is deprecated. We should not use our own deprecated parts, but use the new ones.
| ...writeOptions?.headers, | ||
| }, | ||
| gzipThreshold: writeOptionsOrDefault.gzipThreshold, | ||
| timeout: writeOptions?.timeout ?? this._options.timeout, |
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.
The line you mentioned in the InfluxDBClient doesn't use writeTimeout.
In fact, the line 145 here should be only:
timeout: writeOptions?.timeoutbecause sendOptions are optional.
|
Also, current tests don't validate correct timeout propagation |
Closes #
Proposed Changes
Checklist