[sql-13] sessions: set up for atomic session creation #971
Conversation
ellemouton
added
no-changelog
ellemouton
force-pushed
the
sql13Sessions5
branch
2 times, most recently
from
d55f9a1 to
001da85
Compare
ellemouton
force-pushed
the
sql13Sessions5
branch
2 times, most recently
from
85b049b to
c661a61
Compare
ellemouton
changed the title
| groupIdIndexBkt := sessionBucket.Bucket(groupIDIndexKey) | ||
| if groupIdIndexBkt == nil { | ||
| return ErrDBInitErr | ||
| } | ||
|
|
||
| groupBkt := groupIdIndexBkt.Bucket(session.GroupID[:]) | ||
| if groupBkt == nil { | ||
| return ErrDBInitErr | ||
| } | ||
|
|
||
| sessionIDsBkt := groupBkt.Bucket(sessionIDKey) | ||
| if sessionIDsBkt == nil { | ||
| return ErrDBInitErr | ||
| } |
There was a problem hiding this comment.
newb ques: Can you describe what these 3 Bucket calls are doing? This is the only part of this function that I'm not fully understanding, likely due to my lack of knowledge of bbolt.
There was a problem hiding this comment.
yeah def!
So if we look at the comment at the top of this file which describes the bucket structure:
// sessionBucketKey is the top level bucket where we can find all
// information about sessions. These sessions are indexed by their
// public key.
//
// The session bucket has the following structure:
// session -> <key> -> <serialised session>
// -> id-index -> <session-id> -> key -> <session key>
// -> group -> <group-ID>
// -> group-id-index -> <group-id> -> session-id -> sequence -> <session-id>
sessionBucketKey = []byte("session")
then basically what these bucket calls are doing is indexing into the appropriate sub-buckets so that we can delete the appropriate data :)
There was a problem hiding this comment.
so sessionBucket is the top level session bucket.
- So then
groupIdIndexBkt := sessionBucket.Bucket(groupIDIndexKey)indexes into thegroup-id-indexbucket shown above. groupBkt := groupIdIndexBkt.Bucket(session.GroupID[:])then indexes into the appropriate group ID sub bucket for this groupsessionIDsBkt := groupBkt.Bucket(sessionIDKey)then indexes one layer further
then after this, we need to iterate through the kv pairs in this final bucket to find the appropriate session to delete.
There was a problem hiding this comment.
Thank you for that thorough explanation. Makes sense now, though I can't say I'm a fan of the complexity needed for DB interactions. 🤣
There was a problem hiding this comment.
100% agree - which is why we are SQL-izing and then getting rid of all of this! 🔥
| // StateCreated is the state of a session once it has been fully | ||
| // committed to the Store and is ready to be used. This is the first | ||
| // state of a session. | ||
| StateCreated State = 0 |
There was a problem hiding this comment.
This comment says that StateCreated is the first state for a session but StateReserved also says that it is the temporary initial state. This feels a bit confusing. After reading #979, I understand what these states mean but I don't think these comments alone are clear.
Also, the diagram comment on line 29 is missing StateReserved. Do you plan on fixing these up in the next PR that makes use of the reserved state?
There was a problem hiding this comment.
yeah so this is sort of an intermediary commit that doesnt use the Reserved state yet which is why the diagram does not include it (since it is not possible for a session to be in this state yet). See this commit in the follow up PR which both updates the diagram and the comments :)
session/store_test.go
Outdated
| // Now delete the reserved session and show that it is no longer in the | ||
| // database and no longer in the group ID/session ID index. | ||
| require.NoError(t, db.DeleteReservedSessions()) | ||
|
|
||
| sessions, err = db.ListSessionsByState(StateReserved) | ||
| require.NoError(t, err) | ||
| require.Empty(t, sessions) | ||
|
|
||
| _, err = db.GetGroupID(s5.ID) | ||
| require.ErrorContains(t, err, "no index entry") | ||
|
|
||
| _, err = db.GetSessionIDs(s5.GroupID) | ||
| require.ErrorContains(t, err, "no sessions for group ID") |
There was a problem hiding this comment.
I think it'd also be worth adding test coverage of the behaviour when a session is actually linked to another session as well.
| numSessions++ | ||
|
|
||
| if !bytes.Equal(v, session.ID[:]) { | ||
| return nil |
There was a problem hiding this comment.
Thought this might be worth pointing out for @jamaljsr, so just leaving a comment here, as this always confuses me with kvdb buckets ForEach :). I.e. there's nothing here to address, this looks good but is a bit confusing 🚀!
The return nil doesn't break the ForEach loop here, but rather continues the iteration with the next element of the bucket. Only non-nil errors actually breaks the loop.
ellemouton
force-pushed
the
sql13Sessions5
branch
from
c661a61 to
aafeacd
Compare
For more context, see: #979
In this PR, we just do some set up work for the atomic session creation change.
Here, all we do is:
Reservedsession state and ensure that any sessions in this state are deleted on startup. NOTE: as of this PR, nothing would cause a session to be moved to this state.