feat: Make Consensus Services WritableTopicStore WritableKVState Key to be comparable. #10495
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10495 +/- ##
=============================================
- Coverage 63.49% 63.49% -0.01%
- Complexity 31141 31155 +14
=============================================
Files 3382 3383 +1
Lines 136059 136103 +44
Branches 14180 14187 +7
=============================================
+ Hits 86392 86418 +26
- Misses 46216 46237 +21
+ Partials 3451 3448 -3 ☔ View full report in Codecov by Sentry. |
| * This should be called on each writable store constructor until all other types of Key Comparator are implemented. | ||
| * */ | ||
| @Override | ||
| public final void updateComparator(@NonNull final Comparator<K> comparator) { |
There was a problem hiding this comment.
I think the Comparator should be set during construction and never changed again.
There was a problem hiding this comment.
Yes. Each store is responsible to update their comparator.
There was a problem hiding this comment.
Sorry for not being clear. I would have expected that the Comparator is immutable. There is probably no valid use case where we want to change the Comparator later, or is there? If the Comparator is immutable, the code should reflect that.
There was a problem hiding this comment.
This method has been removed. WritableTopicStore uses a private static final Comparator.
| public final void updateComparator(@NonNull final Comparator<K> comparator) { | ||
| final var tmp = new TreeMap<K, V>(comparator); | ||
| tmp.putAll(modifications); | ||
| this.modifications = tmp; |
There was a problem hiding this comment.
I am afraid I am missing something very essential here. If we update the Comparator, we are not changing the order of the elements in the data structure but the order of modifications, right? Why is this necessary? I assume that if any code is executed deterministically and single-threaded, the modifications should always be in the same order. Why is it necessary to sort them?
There was a problem hiding this comment.
You have a linkedhashmap with put, remove options. If not sorted, how can you guarantee a deterministic order? We care about the writableKVState to be deterministic.
There was a problem hiding this comment.
A TreeMap is created in WritableKVStateBase constructor to enforce the Key order of modification when every service calls its' WritableXXXStore.put().
…te a TreeMap for modifications. Updated all the related Writable/Readable classes for the related change. Added unit tests and updated all the unit tests. Signed-off-by: Iris Simon <[email protected]>
Signed-off-by: Iris Simon <[email protected]>
iwsimon
requested review from
jsync-swirlds,
Neeharika-Sompalli,
tinker-michaelj and
rbair23
| if (comparator != null) modifications = new TreeMap<>(comparator); | ||
| else modifications = new LinkedHashMap<>(); |
There was a problem hiding this comment.
I think we might have missed on additional case here, but it isn't clear how to add it in (darn Erasure behaviors!).
Basically, if K is a Comparable, then we should use the TreeMap in that case as well, without an explicit comparator. We don't have an instance of K to test here, however, so I'm unsure how to test the type (or if we even could).
Probably not worth worrying about at this time, but something perhaps to note as a future issue.
There was a problem hiding this comment.
Agree we should use TreeMap when K is Comparable. Generics Type Erasure is hard to check here. We will make comparator @nonnull when all services have implemented their comparator key. That will cover all cases. We should stick to either Comparator or Comparable, not mix.
Signed-off-by: Iris Simon <[email protected]>
Signed-off-by: Iris Simon <[email protected]>
Signed-off-by: Iris Simon <[email protected]>
|
A PR merged to develop in PBJ that now allows Messages to be marked as "Comparable" (with some limitations). This PR will need to be updated once the proto files are updated and the latest PBJ version is incorporated. Note: Comparable support in PBJ is blocked by a somewhat complex defect in the initial implementation which prevents it from working in services. |
|
Seems this is no more valid. Can we close this @iwsimon ? |
|
Closed as it's not needed any more. |
Description:
Related issue(s):
Fixes #10396
Notes for reviewer:
Discussed this at Jan 9th modularization call, don't pass in Comparator, use PBJ Comparable ID as SortedMap key for WritableKVStateBase.modifications. This has to wait for PBJ Key to implement Comparable.
Checklist