[
https://issues.apache.org/jira/browse/GEODE-8864?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17283317#comment-17283317
]
ASF GitHub Bot commented on GEODE-8864:
---------------------------------------
jhutchison commented on a change in pull request #5954:
URL: https://github.com/apache/geode/pull/5954#discussion_r574772520
##########
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHScanIntegrationTest.java
##########
@@ -336,48 +453,120 @@ 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> originalData = new HashMap<>();
+ originalData.put("field_1", "yellow");
+ originalData.put("field_2", "green");
+ originalData.put("field_3", "grey");
+ jedis.hmset("colors", originalData);
- do {
- result = jedis.hscan("colors", cursor);
- allEntries.addAll(result.getResult());
- cursor = result.getCursor();
- } while (!result.isCompleteIteration());
+ jedis.hdel("colors", "field_3");
+ originalData.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(originalData.entrySet());
}
+
+ /**** Concurrency ***/
+
+ private final int SIZE_OF_INITIAL_HASH_DATA = 100;
+ private Map<String, String> INITIAL_HASH_DATA =
makeEntrySet(SIZE_OF_INITIAL_HASH_DATA);
+ final String HASH_KEY = "key";
+ final String BASE_FIELD = "baseField_";
+
@Test
- public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() {
- assertThatThrownBy(() -> jedis.hscan("a", "18446744073709551616"))
- .hasMessageContaining(ERROR_CURSOR);
+ public void
should_notLoseFields_givenConcurrentThreadsDoingHScansAndChangingValues() {
+ jedis.hset(HASH_KEY, INITIAL_HASH_DATA);
+ final int ITERATION_COUNT = 500;
+
+ new ConcurrentLoopingThreads(ITERATION_COUNT,
+ (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis),
+ (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis2),
+ (i) -> {
+ int fieldSuffix = i % SIZE_OF_INITIAL_HASH_DATA;
+ jedis3.hset(HASH_KEY, BASE_FIELD + fieldSuffix, "new_value_" + i);
+ }).run();
Review comment:
added a new test that checks that data is not altered by concurrent
hscans
##########
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHScanIntegrationTest.java
##########
@@ -336,48 +453,120 @@ 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> originalData = new HashMap<>();
+ originalData.put("field_1", "yellow");
+ originalData.put("field_2", "green");
+ originalData.put("field_3", "grey");
+ jedis.hmset("colors", originalData);
- do {
- result = jedis.hscan("colors", cursor);
- allEntries.addAll(result.getResult());
- cursor = result.getCursor();
- } while (!result.isCompleteIteration());
+ jedis.hdel("colors", "field_3");
+ originalData.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(originalData.entrySet());
}
+
+ /**** Concurrency ***/
+
+ private final int SIZE_OF_INITIAL_HASH_DATA = 100;
+ private Map<String, String> INITIAL_HASH_DATA =
makeEntrySet(SIZE_OF_INITIAL_HASH_DATA);
+ final String HASH_KEY = "key";
+ final String BASE_FIELD = "baseField_";
+
@Test
- public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() {
- assertThatThrownBy(() -> jedis.hscan("a", "18446744073709551616"))
- .hasMessageContaining(ERROR_CURSOR);
+ public void
should_notLoseFields_givenConcurrentThreadsDoingHScansAndChangingValues() {
+ jedis.hset(HASH_KEY, INITIAL_HASH_DATA);
+ final int ITERATION_COUNT = 500;
+
+ new ConcurrentLoopingThreads(ITERATION_COUNT,
+ (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis),
+ (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis2),
+ (i) -> {
+ int fieldSuffix = i % SIZE_OF_INITIAL_HASH_DATA;
+ jedis3.hset(HASH_KEY, BASE_FIELD + fieldSuffix, "new_value_" + i);
+ }).run();
}
+
+
@Test
- public void
givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() {
- assertThatThrownBy(() -> jedis.hscan("a", "-18446744073709551616"))
- .hasMessageContaining(ERROR_CURSOR);
+ public void
should_notLoseDataForConsistentlyPresentFields_givenConcurrentThreadsAddingAndRemovingFields()
{
+
+ Logger logger = LogManager.getLogger("org.apache.geode.redis.internal");
+ Configurator.setAllLevels(logger.getName(), Level.getLevel("DEBUG"));
+ FastLogger.setDelegating(true);
+
+ jedis.hset(HASH_KEY, INITIAL_HASH_DATA);
+ final int ITERATION_COUNT = 5;
+
+ new ConcurrentLoopingThreads(ITERATION_COUNT,
+ (i) -> multipleHScanAndAssertOnContentOfResultSet(jedis),
+ // (i) -> multipleHScanAndAssertOnContentOfResultSet(jedis2),
+ (i) -> {
+ String field = "new_" + BASE_FIELD + i;
+ jedis3.hset(HASH_KEY, field, "whatever");
+ jedis3.hdel(HASH_KEY, field);
+ }).run();
Review comment:
added a new test that checks that data is not altered by concurrent
hscans
----------------------------------------------------------------
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
> finish implementation of Redis HScan Command
> --------------------------------------------
>
> Key: GEODE-8864
> URL: https://issues.apache.org/jira/browse/GEODE-8864
> Project: Geode
> Issue Type: New Feature
> Components: redis
> Reporter: John Hutchison
> Priority: Major
> Labels: pull-request-available
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
