search

scylladb / scylla-manager

The Scylla Manager
https://manager.docs.scylladb.com/stable/
Other
48 stars 33 forks source link

fix(session): close client after session creation #3775

Closed karol-kokoszka closed 3 months ago

karol-kokoszka commented 3 months ago

Fixes #3769

Please make sure that:

Michal-Leszczynski commented 3 months ago

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

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 commented 3 months ago

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 commented 3 months ago

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.