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(session): close client after session creation #3775

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

karol-kokoszka
Copy link
Collaborator

Fixes #3769


Please make sure that:

  • Code is split to commits that address a single change
  • Commit messages are informative
  • Commit titles have module prefix
  • Commit titles have issue nr. suffix

@Michal-Leszczynski
Copy link
Collaborator

I think that there are more places in the code which don't close client/session. Take a look at:

  • client: func (s *Service) ListNodes(ctx context.Context, clusterID uuid.UUID) ([]Node, error)
  • session: func (s *Service) Restore(ctx context.Context, clusterID, taskID, runID uuid.UUID, properties json.RawMessage) error (creation of restore worker)
  • session: func (s *Service) GetTarget(ctx context.Context, clusterID uuid.UUID, properties json.RawMessage) (Target, error) (in repair pkg)

I think that closing client/session is really easy in those places. The bigger problem could be reliably closing cached clients, as cache provider can't just close expired client because it still might be used somewhere. This requires more planning and refactoring, but it's also less problematic since the clients are not recreated multiple times. I guess we can leave it for the future.

@karol-kokoszka
Copy link
Collaborator Author

I think that there are more places in the code which don't close client/session. Take a look at:

client: func (s *Service) ListNodes(ctx context.Context, clusterID uuid.UUID) ([]Node, error)
session: func (s *Service) Restore(ctx context.Context, clusterID, taskID, runID uuid.UUID, properties json.RawMessage) error (creation of restore worker)
session: func (s *Service) GetTarget(ctx context.Context, clusterID uuid.UUID, properties json.RawMessage) (Target, error) (in repair pkg)
I think that closing client/session is really easy in those places.

Yes, but this PR is not the general solution. These clients are taken from cache.
It closes the client just on session creation.

The bigger problem could be reliably closing cached clients, as cache provider can't just close expired client because it still might be used somewhere. This requires more planning and refactoring, but it's also less problematic since the clients are not recreated multiple times. I guess we can leave it for the future.

Exactly, let's leave it for future. The situation with cached client and closing connection on them is something what we had already and we need more general solution.

@Michal-Leszczynski
Copy link
Collaborator

Yes, but this PR is not the general solution. These clients are taken from cache.
It closes the client just on session creation.

But the examples I mentioned above are not taken from cache and can be closed right after they are used, so I would include them in this PR but I would not touch cached clients here.

@karol-kokoszka karol-kokoszka merged commit 2226fb4 into master Mar 29, 2024
19 of 21 checks passed
@karol-kokoszka karol-kokoszka deleted the kk/3769-close-client branch March 29, 2024 13:07
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.

Number of connections established by Scylla Manager outgrows system capabilities
2 participants