-
Notifications
You must be signed in to change notification settings - Fork 453
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
Expose session iterator pools #631
Conversation
client/fetch_state.go
Outdated
MutableSeriesIterators() encoding.MutableSeriesIteratorsPool | ||
MultiReaderIteratorArray() encoding.MultiReaderIteratorArrayPool | ||
ID() ident.Pool | ||
CheckedBytesWrapper() xpool.CheckedBytesWrapperPool | ||
TagDecoder() serialize.TagDecoderPool | ||
} | ||
|
||
// IteratorPools exposes a small subset of iterator pools that are sufficient for clients |
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.
nit: move this to session_pools.go
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.
or better yet, maybe just to encoding/types.go
?
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.
LGTM w/ a nit
client/session.go
Outdated
@@ -1521,6 +1521,10 @@ func (s *session) readConsistencyResult( | |||
return nil | |||
} | |||
|
|||
func (s *session) IteratorPools() (IteratorPools, error) { | |||
return s.pools, nil |
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.
Might want to check that s.pools != nil
? Also are the pools always guaranteed to be available (I can't remember when they get alloc'd). If not we probably have to nil check the specific pools in question too and return an error if nil perhaps?
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.
s.pools is a sessionPools
struct, and isn't itself nilable.
For the pools themselves, I believe they get alloced in session.Open()
, so I think it should be safe unless you get the session
in some weird way. Adding a sanity check would make sense, though.
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.
maybe ensure the session is open then?
s.state.RLock()
defer s.state.RULock()
if s.state.status != statusOpen {
return nil, errSessionStatusNotOpen
}
return s.pools
client/session.go
Outdated
@@ -1521,6 +1521,10 @@ func (s *session) readConsistencyResult( | |||
return nil | |||
} | |||
|
|||
func (s *session) IteratorPools() (encoding.IteratorPools, error) { | |||
return s.pools, nil |
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.
Might want to check that s.pools != nil
? Also are the pools always guaranteed to be available (I can't remember when they get alloc'd). If not we probably have to nil check the specific pools in question too and return an error if nil perhaps?
Codecov Report
@@ Coverage Diff @@
## master #631 +/- ##
==========================================
- Coverage 81.42% 81.34% -0.08%
==========================================
Files 274 274
Lines 24376 24384 +8
==========================================
- Hits 19848 19836 -12
- Misses 3351 3365 +14
- Partials 1177 1183 +6
Continue to review full report at Codecov.
|
No description provided.