feat: Long-term scheduled transactions State API #16144
Conversation
…ization. Add migrations from existing state. Update readable/writable stores to use new states Signed-off-by: Iris Simon <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #16144 +/- ##
==========================================
Coverage 58.14% 58.14%
- Complexity 19848 19854 +6
==========================================
Files 2730 2731 +1
Lines 100084 100138 +54
Branches 10337 10339 +2
==========================================
+ Hits 58189 58223 +34
- Misses 38273 38291 +18
- Partials 3622 3624 +2
... and 8 files with indirect coverage changes |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Signed-off-by: Iris Simon <[email protected]>
Signed-off-by: Iris Simon <[email protected]>
Signed-off-by: Iris Simon <[email protected]>
Signed-off-by: Iris Simon <[email protected]>
Node: HAPI Test (Restart) Results9 files 1 errors 8 suites 7m 34s ⏱️ For more details on these parsing errors, see this check. Results for commit 7ea084b. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
I don't think the change from a list of candidate duplicate schedules to a single candidate duplicate schedule is correct.
The partial "hash" used is not robust, and collisions are quite likely, so we should still have a list (perhaps of ID values instead of whole schedules) keyed to the hash value, and check all of them when a new schedule might be a duplicate.
Edit: The hash function used is better than I thought, so this is incorrect, but see below for an even more effective alternative that avoids needing a hash function entirely.
| private final ReadableKVState<ProtoLong, ScheduleList> schedulesByExpirationSecond; | ||
| private final ReadableKVState<ProtoBytes, ScheduleList> schedulesByStringHash; | ||
| private final ReadableKVState<ProtoLong, ScheduleIdList> scheduleIdsByExpirationSecond; | ||
| private final ReadableKVState<ProtoBytes, Schedule> schedulesByStringHash; |
There was a problem hiding this comment.
String hash values can and will collide (it's only a partial hash, not comprehensive). Changing this to a single schedule will miss duplications. That's why we stored the whole schedule originally; storing a list of ID values might be acceptable as an alternative to storing the whole schedule.
There was a problem hiding this comment.
The reason is explained below.
| private @Nullable Schedule maybeDuplicate( | ||
| @NonNull final Schedule schedule, @Nullable final List<Schedule> duplicates) { | ||
| if (duplicates == null) { | ||
| private @Nullable Schedule maybeDuplicate(@NonNull final Schedule schedule, @Nullable final Schedule duplicate) { |
There was a problem hiding this comment.
This should still handle hash collisions, the hash implementation is not robust enough to guarantee no collisions, so it is entirely likely that multiple schedules will match the hash, and need to be checked for duplication.
Edit: This is incorrect, the hash function used was better than I understood, but we still have a better option that avoids relying on probabilistic factors at all.
There was a problem hiding this comment.
The reason is explained below.
| @@ -56,11 +56,11 @@ public interface ReadableScheduleStore { | |||
| * @return a {@code List<Schedule>} of entries that have the same hash as the provided schedule | |||
| */ | |||
| @Nullable | |||
| List<Schedule> getByEquality(@NonNull Schedule scheduleToMatch); | |||
| Schedule getByEquality(@NonNull Schedule scheduleToMatch); | |||
There was a problem hiding this comment.
This should still be a list (even if it's changed to just be ID values).
The "duplication" hash of the schedule is not enough to know if it's a duplicate, and multiple "candidate" duplicate schedules may have the same "duplication" hash value.
There was a problem hiding this comment.
The reason is explained below.
Thanks for reviewing this PR @jsync-swirlds. After @tinker-michaelj PR #16053, schedule service code has been updated. |
Signed-off-by: Iris Simon <[email protected]>
After multiple discussions, and a discussion in slack about how degenerate inputs are handled by various hash functions, a better approach presents itself (thanks to a suggestion from Leemon). We can take the three fields we compare, and just concatenate those values together for the |
Description:
Define Schema for block stream-friendly state organization.
Add migrations from existing state.
Update readable/writable stores to use new states
Related issue(s):
Fixes #16059
Fixes #16068
Fixes #16069
Notes for reviewer:
Checklist