-
Notifications
You must be signed in to change notification settings - Fork 14
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
Describe pool session management #454
Conversation
client/object_delete.go
Outdated
@@ -61,6 +63,21 @@ func (x PrmObjectDelete) GetSession() (*session.Object, error) { | |||
return &sess, nil | |||
} | |||
|
|||
// IsAutoSessionEnabled is an indicator for Pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's a client
thing, it shouldn't care about pool
.
We already have WithinSession
, it can override pool
behavior for the case when the session is used. We only need a way to explicitly remove the same session. Either it's a WithinSession(nil)
or a separate method (DropSession()
). Then it can be a bool
internally (isSessionSet
) and maybe a special error
from GetSession()
to separate ErrNoSession
and ErrNoSessionExplicitly
. Then all of this is completely generic client
thing, but pool
can use these functions as needed for its magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed DropSession with IgnoreSession, I think this is more neutral. But I tend to think we need to add some description, that this function does nothing for a single client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can do something for a single client! Suppose you've used WithinSession()
somewhere and now you can IgnoreSession()
as well to remove it.
Naming is hard, we know, IgnoreSession
is not bad, but ideally we should somehow signify that this function in many ways is an antonym to WithinSession
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WithinSession
/ IgnoreSession
looks like antipodes. But added some comments to func docs
4349abf
to
c05e575
Compare
8bcaa0b
to
4323119
Compare
client/object_delete.go
Outdated
@@ -25,42 +24,13 @@ var ( | |||
|
|||
// PrmObjectDelete groups optional parameters of ObjectDelete operation. | |||
type PrmObjectDelete struct { | |||
sessionContainer | |||
meta v2session.RequestMetaHeader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a split brain situation with two metas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is obviously excess. Removed
client/session_container.go
Outdated
|
||
// GetSession returns session object. | ||
// | ||
// Returns ErrNoSession err if session wasn't set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ErrNoSession] or [ErrNoSessionExplicitly] if IgnoreSession was used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
4323119
to
949a242
Compare
949a242
to
37c1591
Compare
37c1591
to
a9226b0
Compare
a9226b0
to
da19bbc
Compare
close #449 Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
da19bbc
to
42e02b3
Compare
close #449