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

use the same defaults for session pool config as in the sdk #93

Closed

Conversation

cardamo
Copy link

@cardamo cardamo commented Nov 1, 2024

fixes #92

@nvamelichev nvamelichev requested a review from lavrukov November 1, 2024 19:53
@nvamelichev nvamelichev self-assigned this Nov 1, 2024
@nvamelichev
Copy link
Collaborator

@lavrukov Are you OK with the changes? Making an explicit default for session pool min size, instead of having min size=max size; and lowering idle time and default min/max pool size seem quite reasonable to me, and also align YOJ with the YDB Java SDK behavior.

I believe that anybody on the Right has either configured YDB session pool explicitly (if they care) or won't notice and/or care if the defaults change slightly.

@nvamelichev
Copy link
Collaborator

nvamelichev commented Nov 2, 2024

@cardamo Artem, let's take the pool defaults from SessionPoolOptions.DEFAULT constant in the SDK (it's public) if possible.
Otherwise LGTM

@cardamo
Copy link
Author

cardamo commented Nov 2, 2024

nope, getters in tech.ydb.table.impl.pool.SessionPoolOptions are package-private 🤷‍♂️

@nvamelichev nvamelichev self-requested a review November 29, 2024 14:57
Copy link
Collaborator

@nvamelichev nvamelichev left a comment

Choose a reason for hiding this comment

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

👎 After discussions with @lavrukov, I'm declining this PR as too risky for Right's Cloud Java (sorry).

  • Short-term: We probably should change/enforce defaults in our libraries, instead.
  • Also of note: Why the hell does YDB Java SDK not check that sessions are alive? Do we maybe need to make improvements there, or do we maybe need to enable some YDB Java SDK feature that will check sessions for liveness?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use the same default values for session pool as in ydb sdk
2 participants