-
Notifications
You must be signed in to change notification settings - Fork 88
indexer: Cache scanner table #960
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
base: main
Are you sure you want to change the base?
Conversation
8620166 to
33f440f
Compare
294bced to
fd5f4e9
Compare
3a98167 to
69a3557
Compare
During the indexing cycle many operations try to read information from the scanner table, this can occasionally lead to resource contention depending on the DB. This reads the table into memory when the indexer store is instanciated and uses when asked for scanner info. Signed-off-by: crozzy <[email protected]>
2 years ago??? |
RTann
left a comment
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.
So let's say we are in the process of updating the Indexer and/or Matcher version(s) to add or delete a scanner.
Add: I think this is ok. The older versions won't even look for it (I believe)
Delete: The older versions will still claim the scanner exists, though I guess it'll still fail the next part of the scan process, maybe?
Can you add any commentary to potential race scenarios to see if we are ok with the possibilities?
| ` | ||
|
|
||
| func (s *IndexerStore) populateScanners(ctx context.Context) error { | ||
| s.scanners = make(map[string]int64) |
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.
This is already done in indexer_store.go
| return s.populateScanners(ctx) | ||
| } | ||
|
|
||
| const selectAllScanner = ` |
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: selectAllScanners?
| if err != nil { | ||
| return fmt.Errorf("failed to retrieve scanners: %w", err) | ||
| } | ||
| for rows.Next() { |
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.
I think we still need a defer rows.Close() (I don't think it'll close if rows.Scan fails) and a rows.Err() check, too
| type IndexerStore struct { | ||
| pool *pgxpool.Pool | ||
| pool *pgxpool.Pool | ||
| scanners map[string]int64 |
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.
this may be a decent candidate for map[unique.Handle[string]]int64 since I think this map will be rather static once it's initially populated
| registerScannerDuration.WithLabelValues("insert").Observe(time.Since(start).Seconds()) | ||
| } | ||
|
|
||
| return s.populateScanners(ctx) |
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.
is it assumed that RegisterScanners is called only once and in the very beginning of the process?
| scanner; | ||
| ` | ||
|
|
||
| func (s *IndexerStore) populateScanners(ctx context.Context) error { |
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: initScanners
During the indexing cycle many operations try to read information from the scanner table, this can occasionally lead to resource contention depending on the DB. This reads the table into memory when the indexer store is instanciated and uses when asked for scanner info.