fix(spanner): mark SessionPoolConfig.MaxBurst deprecated #4115
Conversation
google-cla
bot
added
the
cla: yes
|
@olavloite Can we remove the related test if we deprecate |
spanner/session.go
Outdated
| @@ -409,7 +409,8 @@ type SessionPoolConfig struct { | |||
| // Defaults to 0. | |||
| MaxIdle uint64 | |||
|
|
|||
| // MaxBurst is the maximum number of concurrent session creation requests. | |||
| // (Deprecated) MaxBurst is the maximum number of concurrent session | |||
| // creation requests. | |||
There was a problem hiding this comment.
nit: Could you format this deprecation notice in this form: https://github.com/golang/go/wiki/Deprecated#examples
This way linters will pick it up in developers code and issue a warning. Also, is there something people should migrate to instead?
There was a problem hiding this comment.
Thanks for the link. Will do.
@olavloite Do we still support MaxBurst? or something they can migrate to?
There was a problem hiding this comment.
From my understanding, the sessions will be created in a batch and the size of the batch (max burst) is decided by SessionPoolConfig.incStep. However, SessionPoolConfig.incStep is not configurable.
There was a problem hiding this comment.
We do not support MaxBurst anymore, and there is also nothing that replaces it. SessionPool.incStep will determine the number of sessions that is created in each BatchCreateSessions RPC invocation, and is not (externally) configurable, as the default value has been shown to be a reasonable configuration for all tested scenarios. Also, SessionPool.incStep is different from MaxBurst. MaxBurst was used to limit the number of sessions that the session pool could create within a time frame. This was an early safety valve to prevent a client from overwhelming the backend if a large number of sessions was suddenly needed. The session pool would then pause the creation of sessions for a while. Such a pause is no longer needed and the implementation has been removed from the pool.
spanner/session.go
Outdated
| @@ -409,7 +409,8 @@ type SessionPoolConfig struct { | |||
| // Defaults to 0. | |||
| MaxIdle uint64 | |||
|
|
|||
| // MaxBurst is the maximum number of concurrent session creation requests. | |||
| // (Deprecated) MaxBurst is the maximum number of concurrent session | |||
| // creation requests. | |||
There was a problem hiding this comment.
We do not support MaxBurst anymore, and there is also nothing that replaces it. SessionPool.incStep will determine the number of sessions that is created in each BatchCreateSessions RPC invocation, and is not (externally) configurable, as the default value has been shown to be a reasonable configuration for all tested scenarios. Also, SessionPool.incStep is different from MaxBurst. MaxBurst was used to limit the number of sessions that the session pool could create within a time frame. This was an early safety valve to prevent a client from overwhelming the backend if a large number of sessions was suddenly needed. The session pool would then pause the creation of sessions for a while. Such a pause is no longer needed and the implementation has been removed from the pool.
|
@olavloite Thanks for the explanation. Please take a look at the latest change again (I put your explanation into the comment). |
Fixes #4089