ParticipationRegistry Cache #2769
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/partkey #2769 +/- ##
===================================================
+ Coverage 47.41% 47.46% +0.04%
===================================================
Files 351 352 +1
Lines 56498 56747 +249
===================================================
+ Hits 26790 26934 +144
- Misses 26690 26786 +96
- Partials 3018 3027 +9
Continue to review full report at Codecov.
|
tsachiherman
left a comment
tsachiherman
left a comment
There was a problem hiding this comment.
Overall looks great. Few small requests.
- add logger - ParcipationRecord Duplicate function - remove unused get function - more tests regarding cache desynchronization
| if _, ok := db.cache[id]; ok { | ||
| return ParticipationID{}, ErrAlreadyInserted | ||
| } | ||
|
|
There was a problem hiding this comment.
I think you can dodge around making the second query to get the primary key by analyzing the result from the first query ( which, btw, need to have it's result being tested ).
The result in the first query would have LastInsertId, which is the primary key you're after.
There was a problem hiding this comment.
Nice tip about LastInsertId!
Is an insert that doesn't have an error insufficient error checking to know that an insert was performed? This is really tedious stuff:
result, err := tx.Exec(
insertKeysetQuery,
id[:],
record.Parent[:],
record.FirstValid,
record.LastValid,
record.KeyDilution)
if err != nil {
return fmt.Errorf("unable to insert keyset: %w", err)
}
rows, err := result.RowsAffected()
if err != nil {
return fmt.Errorf("unable to insert keyset: %w", err)
}
if rows != 1 {
return fmt.Errorf("unexpected number of rows")
}
pk, err := result.LastInsertId()
if err != nil {
return fmt.Errorf("unable to insert keyset: %w", err)
}
There was a problem hiding this comment.
The number of rows affected by an insert could be greater than one. I don't know if in sqlite there is such a use case, but in Postgres you can have ON CONFLICT which would make it a nop.
| db.mutex.Lock() | ||
| defer db.mutex.Unlock() | ||
|
|
||
| matches := make([]ParticipationRecord, 0) |
There was a problem hiding this comment.
using
matches := make([]ParticipationRecord, 0, len(db.cache))would avoid potential reallocation.
There was a problem hiding this comment.
Actually, we can do better. Anything but 1 match is an error so I can exit in the loop if a second is found.
3 participants
Summary
Add a cache in the ParticipationRegistry to optimize asynchronous data access and updates.
Test Plan
Update unit tests.