Make CachedQueryEntry to implement IDS #18238
Conversation
Fixes hazelcast#18131 CachedQueryEntry is an output of maxBy/minBy aggregations when aggregating entries matching a predicate which was evaluated by using an index. The fix is not straightforward, there is some extra complexity so recording here for future generations: This changeset makes CachedQueryEntry to use the same Class ID as LazyMapEntry which already is serializable. This means why you serialize CachedQueryEntry and deserialize again you will get an instance of LazyMapEntry. This is already confusing enough, but it gets worst: LazyMapEntry is subclass of CachedQueryEntry! So you might be asking: What is going on? The explanation is not simple, but I will try anyway: I could make the CachedQueryEntry to use a separate Class ID. But this means all clients would have to be updated as well. As they would not know this new ID. Still, this on its own would not justify using a class ID belonging to a different class. This is the contract of LazyMapEntry: "When you serialize and deserialize LazyMapEntry it loses its "lazy" properties." After deserialize it behaves as if it was a plain SimpleMapEntry and that's exactly what we need. TODO: Write sed-de test for CacheQueryEntry
also leftover parameter removed
|
Instead of having to document an unexpected situation as in here: https://github.com/hazelcast/hazelcast/pull/18238/files#diff-704432b994852652577c49594ec841d6f15265fbc703320d1a64f42c825dd905R224 What about doing something like these:
Gain is these classes will be aligned with their intention of usage. |
|
@jerrinot what's the state of this fix? I see it's draft, there's a TODO and Ahmet posted a comment. Can you finalise the PR? |
|
@ahmetmircik that's an interesting idea! do you want to do a change like this in a minor release? wouldnt it be better to wait for the 5.0 release? I know technically it does not break compatibility (yay, IDS!), but it still seem to be quite surprising. |
|
@jerrinot Of course it is up to you but if no big obstacle, i prefer it to workarounds. Workarounds seem more surprising in this context. |
|
The job Click to expand the log file-------------------------- -------TEST FAILURE------- -------------------------- [INFO] Results: [INFO] [ERROR] Failures: [ERROR] CachedQueryEntryTest.testDeserialization:221 expected:<1476017569> but was:<112004910> [INFO] [ERROR] Tests run: 36301, Failures: 1, Errors: 0, Skipped: 988 [INFO] |
jerrinot
force-pushed
the
fixes/cache-query-entry-ids/master
branch
from
d0139f5
to
1842123
Compare
Fixes #18131
EE: https://github.com/hazelcast/hazelcast-enterprise/pull/3973
CachedQueryEntry is an output of maxBy/minBy aggregations
when aggregating entries matching a predicate which was
evaluated by using an index.
The fix is not straightforward, there is some extra complexity
so recording here for future generations:
This changeset makes CachedQueryEntry to use the same Class ID
as LazyMapEntry which already is serializable. This means why you
serialize CachedQueryEntry and deserialize again you will get
an instance of LazyMapEntry. This is already confusing enough,
but it gets worst: LazyMapEntry is subclass of CachedQueryEntry!
So you might be asking: What is going on?
The explanation is not simple, but I will try anyway:
I could make the CachedQueryEntry to use a separate Class ID. But this means
all clients would have to be updated as well. As they would not know this new ID.
Still, this on its own would not justify using a class ID belonging to
a different class. This is the contract of LazyMapEntry: "When you serialize
and deserialize LazyMapEntry it loses its "lazy" properties." After deserialize
it behaves as if it was a plain SimpleMapEntry and that's exactly what we need.