move session store to storage.SessionDatabase #2475
Conversation
storage/interface.go
Outdated
| // SessionDatabase is an in memory database that holds session data on a KV basis. | ||
| // Keys could be access tokens, nonce's, authorization codes, etc. | ||
| // All entries are stored with a TTL, so they will be removed automatically. | ||
| type SessionDatabase interface { | ||
| // GetStore returns a SessionStore with the given keys as key prefixes. | ||
| // The keys are used to logically partition the store. | ||
| // The TTL is the time-to-live for the entries in the store. | ||
| GetStore(ttl time.Duration, keys ...string) SessionStore | ||
| // Close stops any background processes and closes the database. | ||
| Close() | ||
| } |
There was a problem hiding this comment.
I don't see the use for this interface. The storage engine should return stores, the underlying DB is the responsibility of the storage engine. A SessionStore can have method to set ttl and keys (rename to prefixes)?
(Handling of store closure should also be the responsibility of the storage engine. The storage engine is started before any store is needed, so it also closed after the stores will be needed.)
There was a problem hiding this comment.
close has been made private. The storage engine does manage this DB, there's just one and there should not be any need for another one in parallel.
storage/session.go
Outdated
| // SessionDatabase is an in memory database that holds session data on a KV basis. | ||
| // Keys could be access tokens, nonce's, authorization codes, etc. | ||
| // All entries are stored with a TTL, so they will be removed automatically. | ||
| type inMemorySessionDatabase struct { |
There was a problem hiding this comment.
Should this be a wrapper on a stoabs.Store to limit our store types and simplify migration? Redis supports expiring entries since it is designed as caching layer, but it can also be an in memory bbolt store.
The SessionStore returned by GetStore methods smells a lot like a shelf with a TTL() stoabs.TxOption
There was a problem hiding this comment.
I agree in general; who's going to use this? It feels to detached from the current storage functionality and types; (Provider providing KVStores, and SessionDatabase providing SessionStores).
I think you could even build this on top of KVStore (and use in-memory BBolt db indeed)?
There was a problem hiding this comment.
It must be detached from the current storage since it has a totally different goal. Session storage must never be written to disk and must be clustered (real time consistency, not eventual through copying). Therefore there's no logic in using bbolt or the KVStore abstraction. It's placed in storage just for clustering storage related things.
There was a problem hiding this comment.
The storage engine isn't exclusively for KVStore, any SQL client could be placed in storage as well.
There was a problem hiding this comment.
Although session storage is often persistable (to handle continuance after crash/reboot/update), we probably don't need it (at least not at this point). The simplest solution is also the easiest to migrate later on, so I'm fine with a separate in-memory store, as long as the interface stays simple.
| // SessionStore is a key-value store that holds session data. | ||
| // The SessionStore is a wrapper for the SessionDatabase, it automatically adds prefixes for logical partitions. | ||
| // It uses JSON marshalling to store the entries. | ||
| type SessionStore interface { |
There was a problem hiding this comment.
should include ctx on all methods if there is going to be a version that is not in memory
There was a problem hiding this comment.
That could be done when an implementation is added that supports it. Until then this would add unused parameters.
storage/session.go
Outdated
| // SessionDatabase is an in memory database that holds session data on a KV basis. | ||
| // Keys could be access tokens, nonce's, authorization codes, etc. | ||
| // All entries are stored with a TTL, so they will be removed automatically. | ||
| type inMemorySessionDatabase struct { |
There was a problem hiding this comment.
I agree in general; who's going to use this? It feels to detached from the current storage functionality and types; (Provider providing KVStores, and SessionDatabase providing SessionStores).
I think you could even build this on top of KVStore (and use in-memory BBolt db indeed)?
woutslakhorst
force-pushed
the
feature/2391/session_storage
branch
from
18775e4 to
cbbc851
Compare
closes #2391
This moves the in memory session store to the storage engine. In a later stage this makes it easier to change the in memory variant to a clustered variant.
The generic one includes expiry out-of-the-box.
It uses the JSON marshalling style of set and get for generics.