GEODE-9018: Fix ExtendedNumericComparator for NULL and UNDEFINED comp… #6110
Conversation
albertogpz
requested review from
agingade,
DonalEvans and
nabarunnag
as code owners
| p1.positions.put("SUN", "yes"); | ||
| region.put("KEY-" + 1, p1); | ||
|
|
||
| // null value for positions['SUN'] |
There was a problem hiding this comment.
This comment (and some others in this test) are inaccurate
| import org.apache.geode.cache.query.internal.index.IndexManager; | ||
|
|
||
| public class ExtendedNumericComparatorTest { | ||
| Comparator comparator = TypeUtils.getExtendedNumericComparator(); |
There was a problem hiding this comment.
Warnings in this class can be fixed by using Comparator<Object> comparator = uncheckedCast(TypeUtils.getExtendedNumericComparator()); here
| * for the key | ||
| */ | ||
| @Test | ||
| public void testNullAndUndefinedValuesForMapKeyInCompactRangeIndex() throws Exception { |
There was a problem hiding this comment.
This test passes even with the changes in ExtendedNumericComparator reverted. Would it be possible to write a test that exercises the new behaviour?
There was a problem hiding this comment.
Actually, the test without the changes is flaky. If you run it several times you will see it failing a certain percentage of times.
There was a problem hiding this comment.
Would it be possible to write a test that wasn't flaky when it failed? If something changed in future that made this test fail, it would be best if it failed immediately rather than randomly some time after the breaking change was made.
There was a problem hiding this comment.
Unfortunately, I have not found a way.
The flakyness has to do with some randomness in the implementation of the ConcurrentSkipListMap::putIfAbsent() method invoked from MemoryIndexStore::updateMapping() on the valueToEntriesMap member that seems to traverse the Map in different order in different executions.
With the bug in the Comparator, when adding an entry with UNDEFINED key to the valueToEntriesMap, when there was already an entry with that key in the Map, sometimes that was not detected (an entry with NullToken was found first and that prevented looking for the UNDEFINED key in another entry) and several entries with the UNDEFINED key ended up in the Map.
...grationTest/java/org/apache/geode/cache/query/internal/index/CompactRangeIndexJUnitTest.java
Show resolved
Hide resolved
apache#6110) * GEODE-9018: Fix ExtendedNumericComparator for NULL and UNDEFINED comparison * GEODE-9018: Changes after review * GEODE-9018: Added checks in test case after review
…arison
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.