jhutchison commented on a change in pull request #5954: URL: https://github.com/apache/geode/pull/5954#discussion_r583843017
########## File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHScanIntegrationTest.java ########## @@ -336,48 +441,136 @@ public void givenMultipleCountsAndMatches_returnsEntriesMatchingLastMatchParamet } @Test - public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { - Map<String, String> entryMap = new HashMap<>(); - entryMap.put("1", "yellow"); - entryMap.put("2", "green"); - entryMap.put("3", "orange"); - jedis.hmset("colors", entryMap); + public void should_notReturnValue_givenValueWasRemovedBeforeHSCANISCalled() { - String cursor = "-100"; - ScanResult<Map.Entry<String, String>> result; - List<Map.Entry<String, String>> allEntries = new ArrayList<>(); + Map<String, String> data = new HashMap<>(); + data.put("field_1", "yellow"); + data.put("field_2", "green"); + data.put("field_3", "grey"); + jedis.hmset("colors", data); - do { - result = jedis.hscan("colors", cursor); - allEntries.addAll(result.getResult()); - cursor = result.getCursor(); - } while (!result.isCompleteIteration()); + jedis.hdel("colors", "field_3"); + data.remove("field_3"); - assertThat(allEntries).hasSize(3); - assertThat(new HashSet<>(allEntries)).isEqualTo(entryMap.entrySet()); + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.hget("colors", "field_3")).isNull()); + + ScanResult<Map.Entry<String, String>> result = jedis.hscan("colors", "0"); + + assertThat(new HashSet<>(result.getResult())) + .containsExactlyInAnyOrderElementsOf(data.entrySet()); } @Test - public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.hscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + public void should_retun_not_error_given_non_zero_cursor_on_first_call() { + + Map<String, String> data = new HashMap<>(); + data.put("field_1", "yellow"); + data.put("field_2", "green"); + data.put("field_3", "grey"); + jedis.hmset("colors", data); + + + ScanResult<Map.Entry<String, String>> result = jedis.hscan("colors", "5"); + + assertThat(new HashSet<>(result.getResult())) + .containsExactlyInAnyOrderElementsOf(data.entrySet()); } + /**** Concurrency ***/ + + private final int SIZE_OF_INITIAL_HASH_DATA = 100; + final String HASH_KEY = "key"; + final String BASE_FIELD = "baseField_"; + @Test - public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.hscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + public void should_notLoseFields_givenConcurrentThreadsDoingHScansAndChangingValues() { + final Map<String, String> INITIAL_HASH_DATA = makeEntrySet(SIZE_OF_INITIAL_HASH_DATA); + jedis.hset(HASH_KEY, INITIAL_HASH_DATA); + final int ITERATION_COUNT = 500; + + new ConcurrentLoopingThreads(ITERATION_COUNT, + (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis, INITIAL_HASH_DATA), + (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis2, INITIAL_HASH_DATA), + (i) -> { + int fieldSuffix = i % SIZE_OF_INITIAL_HASH_DATA; + jedis3.hset(HASH_KEY, BASE_FIELD + fieldSuffix, "new_value_" + i); + }).run(); + } @Test - public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.hset("a", "1", "green"); - ScanParams scanParams = new ScanParams(); - scanParams.count(1); - scanParams.match("\\p"); + public void should_notLoseKeysForConsistentlyPresentFields_givenConcurrentThreadsAddingAndRemovingFields() { + final Map<String, String> INITIAL_HASH_DATA = makeEntrySet(SIZE_OF_INITIAL_HASH_DATA); + jedis.hset(HASH_KEY, INITIAL_HASH_DATA); + final int ITERATION_COUNT = 500; + + new ConcurrentLoopingThreads(ITERATION_COUNT, + (i) -> multipleHScanAndAssertOnContentOfResultSet(jedis, INITIAL_HASH_DATA), + (i) -> multipleHScanAndAssertOnContentOfResultSet(jedis2, INITIAL_HASH_DATA), + (i) -> { + String field = "new_" + BASE_FIELD + i; + jedis3.hset(HASH_KEY, field, "whatever"); + jedis3.hdel(HASH_KEY, field); + }).run(); - ScanResult<Map.Entry<String, String>> result = jedis.hscan("a", "0", scanParams); + } - assertThat(result.getResult()).isEmpty(); + @Test + public void should_notAlterUnderlyingData_givenMultipleConcurrentHscans() { + final Map<String, String> INITIAL_HASH_DATA = makeEntrySet(SIZE_OF_INITIAL_HASH_DATA); + jedis.hset(HASH_KEY, INITIAL_HASH_DATA); + final int ITERATION_COUNT = 500; + + new ConcurrentLoopingThreads(ITERATION_COUNT, + (i) -> multipleHScanAndAssertOnContentOfResultSet(jedis, INITIAL_HASH_DATA), + (i) -> multipleHScanAndAssertOnContentOfResultSet(jedis2, INITIAL_HASH_DATA)); + + INITIAL_HASH_DATA.forEach((field, value) -> { + assertThat(jedis3.hget(HASH_KEY, field).equals(value)); + }); + + } + + private void multipleHScanAndAssertOnContentOfResultSet(Jedis jedis, + final Map<String, String> intialHashData) { Review comment: done ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org