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(spanner): mark SessionPoolConfig.MaxBurst deprecated #4115

Merged
merged 4 commits into from May 19, 2021

Conversation

hengfengli
Copy link
Contributor

@hengfengli hengfengli commented May 18, 2021

Fixes #4089

@hengfengli hengfengli added the api: spanner Issues related to the Cloud Spanner API. label May 18, 2021
@hengfengli hengfengli requested a review from olavloite May 18, 2021
@hengfengli hengfengli self-assigned this May 18, 2021
@hengfengli hengfengli requested review from skuruppu and a team as code owners May 18, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 18, 2021
@hengfengli
Copy link
Contributor Author

hengfengli commented May 18, 2021

@olavloite Can we remove the related test if we deprecate SessionPoolConfig.MaxBurst? It seems the test still passes.

@@ -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.
Copy link
Member

@codyoss codyoss May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@hengfengli hengfengli May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link. Will do.

@olavloite Do we still support MaxBurst? or something they can migrate to?

Copy link
Contributor Author

@hengfengli hengfengli May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@olavloite olavloite May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@olavloite olavloite May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hengfengli
Copy link
Contributor Author

hengfengli commented May 19, 2021

@olavloite Thanks for the explanation. Please take a look at the latest change again (I put your explanation into the comment).

@skuruppu skuruppu merged commit d60a686 into googleapis:master May 19, 2021
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Cloud Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: mark SessionPoolConfig.MaxBurst as unused
4 participants