[sql-12] sessions: make ListSession methods SQL ready - #970
Conversation
ellemouton
added
no-changelog
a586a55 to
9c48b0b
Compare
ellemouton
changed the title
6cc7863 to
a067bce
Compare
jamaljsr
left a comment
jamaljsr
left a comment
There was a problem hiding this comment.
tACK LGTM. I'm loving this micro PRs 🙌
I ran the TW e2e tests against a litd node running this branch to confirm that everything still works and it's all green. Great work 💪
I just have one questions, but otherwise approved.
| // We only start non-revoked, non-expired LiT sessions. Everything else | ||
| // we just skip. | ||
| if sess.State != session.StateInUse && | ||
| sess.State != session.StateCreated { | ||
|
|
||
| log.Debugf("Not resuming session %x with state %d", pubKeyBytes, | ||
| sess.State) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Is it safe to assume that the passed in session will not have an revoked/expired state? I see that all current calls to resumeSession ensure that the sesion is in a valid state, but I'm thinking about future code that may not have these safeguards in place.
If you think it's best to still remove this, then the function comment should be updated.
There was a problem hiding this comment.
cool yeah currently all the session passed to this method will already be in the Created/InUse state since it is either called from start which now only fetches sessions in these states or it is called on session creation which will defs have the CreatedState.
GOod call - will update the comment 👍
a067bce to
e94adc3
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Niiiiice 🚀! Very clean PR 🔥
Laaving a few non-blocking suggestions that I'll let you decide if you think they're worth implementing or not.
|
|
||
| // listSessions returns all sessions currently known to the store that pass the | ||
| // given filter function. | ||
| func (db *BoltStore) listSessions(filterFn func(s *Session) bool) ([]*Session, error) { |
| s1 := newSession(t, db, clock, "session 1") | ||
| clock.SetTime(testTime.Add(time.Second)) | ||
| s2 := newSession(t, db, clock, "session 2") | ||
| clock.SetTime(testTime.Add(2 * time.Second)) | ||
| s3 := newSession(t, db, clock, "session 3", withType(TypeAutopilot)) |
There was a problem hiding this comment.
Not sure if this really tests that the sessions are sorted by creation time, as one could argue they're just sorted by the order they were added here. So I would ensure that one of the latter sessions we add, say s3, has a creation time prior to s2.
There was a problem hiding this comment.
the main reason for incrementing the timestamps is to have deterministic results from ListSessions. We are just testing ListSessions as a whole, not that the results are ordered.
| // | ||
| // NOTE: this is part of the Store interface. | ||
| func (db *BoltStore) ListSessions(filterFn func(s *Session) bool) ([]*Session, error) { | ||
| func (db *BoltStore) ListSessions(states ...State) ([]*Session, error) { |
There was a problem hiding this comment.
nit:
Given that we have a separate ListSessionsByType function, maybe it'd make sense to have a
ListSessionsByState function separately that takes the states ...State arg, and then let the ListSessions itself take no args an have no filtering?
IMO it just feels a bit weird that the plain ListSessions func does take filtering State(s) args, while we have another separate function ListSessionsByType function.
There was a problem hiding this comment.
updated as per suggestion
So that we can add more modifiers for future tests.
Sorted by creation time. Also add a test to cover this.
And use it to replace one call to ListSessions which uses a filter function which would be inefficient in SQL land.
And replace with ListAllSessions since no callers of ListSessions currently make use of the filter function.
Using the new ListSessions by type method, we no longer need to fetch and iterate through all our sessions on start up to figure out which ones to spin up.
e94adc3 to
14cb0be
Compare
3 participants
In this PR, we make various changes to the ListSessions method of the Store interface such that it
can better be implemented by a SQL backend. See commit messages for more details.
Part of #966