ADR-026 Home Database Cache #618
Conversation
| @@ -507,3 +514,21 @@ func (p *Pool) deactivateWriter(serverName string, db string) { | |||
| p.log.Debugf(log.Pool, p.logId, "Deactivating writer %s for database %s", serverName, db) | |||
| p.router.InvalidateWriter(db, serverName) | |||
| } | |||
|
|
|||
| func (p *Pool) updateCacheState() { | |||
There was a problem hiding this comment.
Functionally correct as far as I can tell. But this has optimization potential. The server could instead track the number of connections with and without SSR using two atomics. This is faster beacuse
- not holding a lock on the whole server while iterating over all connections
- not performing the update operation after every pool borrow/return, but only when a connection is being opened/closed
| @@ -295,12 +302,12 @@ type sessionRouter interface { | |||
| // note: bookmarks are lazily supplied, only when a new routing table needs to be fetched | |||
| // this is needed because custom bookmark managers may provide bookmarks from external systems | |||
| // they should not be called when it is not needed (e.g. when a routing table is cached) | |||
| GetOrUpdateReaders(ctx context.Context, bookmarks func(context.Context) ([]string, error), database string, auth *idb.ReAuthToken, boltLogger log.BoltLogger) ([]string, error) | |||
| GetOrUpdateReaders(ctx context.Context, bookmarks func(context.Context) ([]string, error), database string, auth *idb.ReAuthToken, boltLogger log.BoltLogger, useHomeDbGuess bool) ([]string, error) | |||
There was a problem hiding this comment.
Potential for refactoring: the useHomeDbGuess flag is probably better called isHomeDbGuess or just guessedDb or such. Further, I think it would go great together with the database string into a single struct as this data is always passed around together until eventually it's broken apart and only the string (or "") is passed further down the stack.
| const DefaultCacheMaxSize = 1000 | ||
|
|
||
| // cachePruneFraction defines the fraction of entries to prune when the cache exceeds its size. | ||
| const cachePruneFraction = 0.1 |
There was a problem hiding this comment.
The ADR states that (and how) cachePruneFraction should depend on the size of the cache. Currently, the cache size is fixed, but the cache is written in a way that it can easily be changed from the outside (NewCache takes a maxSize).
In order for this relation between the numbers not to get lost, I recommend to do one of the following:
- compute the prune factor dynamically in
NewCache - make
NewCachenot take amaxSize, but instead always useDefaultCacheMaxSize. In this case, you can pre-computecachePruneFractionand use a constant like it's right now. When using this option, I suggest to leave a comment at the const definition that explains how to update the value shouldDefaultCacheMaxSizebe changed (i.e., put the formula there).
| if !c.IsEnabled() { | ||
| return | ||
| } |
There was a problem hiding this comment.
If I read the code right, when SSR is disabled/unknown, the cache is not updated even if we learn about about a user -> homeDD mapping. We decided to track the homeDBs regardless of SSR availability (s. ADR).
I thought there's a TestKit test for that 👀 If not, could you add one?
| } | ||
|
|
||
| // InvalidateDatabase removes all entries that reference a specific database. | ||
| func (c *Cache) InvalidateDatabase(database string) { |
There was a problem hiding this comment.
Nit: dead code (unused method)
| @@ -725,7 +800,7 @@ func (s *sessionWithContext) getServerInfo(ctx context.Context) (ServerInfo, err | |||
| if err := s.resolveHomeDatabase(ctx); err != nil { | |||
| return nil, errorutil.WrapError(err) | |||
| } | |||
| _, err := s.getOrUpdateServers(ctx, idb.ReadMode) | |||
| _, err := s.getOrUpdateServers(ctx, idb.ReadMode, false) | |||
There was a problem hiding this comment.
getServerInfo, is no exception from this new feature. It, too, can and should make use of the home db cache.
Worth writing a TestKit test for, I think 📝
N.B. technically, verifyAuthentication should also make use of the cache. But the way it's exposed to the user this function will only be called with the session configured with "system" as DB. But that again is making assumptions cross concern boundaries, which is a recipe for regressions (spooky action at a distance).
| return | ||
| } | ||
|
|
||
| key, _ := computeCacheKey(ctx, s.auth, s.cache, s.config.ImpersonatedUser) |
There was a problem hiding this comment.
👀 I spy with my little eye a swallowed error. Is this on purpose? Do we want to continue with a key that might be ""?
| if v, ok := authTokenMap["scheme"].(string); ok { | ||
| scheme = v | ||
| } |
There was a problem hiding this comment.
Refactoring potential: 4 times basically the same 3 lines. Something along the lines of
func mapGetString(data map[string]any, key string) string {
out, _ := data[key].(string)
return out
}| // It returns a list of servers, a boolean indicating whether the home database guess was used, and an error if resolution fails. | ||
| func (s *sessionWithContext) getServerList(ctx context.Context, mode idb.AccessMode) ([]string, bool, error) { | ||
| if s.config.DatabaseName != "" { | ||
| if err := s.resolveHomeDatabase(ctx); err != nil { |
There was a problem hiding this comment.
Why are we calling resolveHomeDatabase when there is an explicit database configured on the session?
| key, _ := computeCacheKey(ctx, s.auth, s.cache, s.config.ImpersonatedUser) | ||
| s.cache.Set(key, database) | ||
|
|
||
| s.log.Debugf(log.Session, s.logId, "Resolved home database, uses db '%s'", database) |
There was a problem hiding this comment.
You know I like myself some good logging 😄 Optional suggestion: a bit more logging in the session. E.g., when we decide to explicitly resolve the home database, ... It's up to you and ofc. should't become too noisy.
Added support for Bolt 5.8, which introduces a couple of useful features. The server now echoes back the resolved home/default database when starting a transaction, along with a connection hint indicating whether server-side routing (SSR) is enabled. If SSR is on, the echoed database can be used with an optimistic home database cache to predict a client-side route (optimistic routing), falling back to SSR if the guess is wrong. This can eliminate a round-trip, but only if all the following conditions are met: