jdeppe-pivotal commented on a change in pull request #5954:
URL: https://github.com/apache/geode/pull/5954#discussion_r577107334
##########
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHScanIntegrationTest.java
##########
@@ -219,12 +301,13 @@ public void
givenCount_returnsAllEntriesWithoutDuplicates() {
cursor = result.getCursor();
} while (!result.isCompleteIteration());
- assertThat(allEntries).hasSize(3);
- assertThat(new HashSet<>(allEntries)).isEqualTo(entryMap.entrySet());
+ assertThat(allEntries.size()).isCloseTo(3, Offset.offset(1));
}
+ @Ignore
Review comment:
Unless there is functionality here that will eventually get fixed, this
test should just be removed. It might be better to simply assert that it is not
an error to provide multiple `COUNT` arguments.
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -46,19 +52,71 @@
public class RedisHash extends AbstractRedisData {
public static final RedisHash NULL_REDIS_HASH = new NullRedisHash();
private HashMap<ByteArrayWrapper, ByteArrayWrapper> hash;
+ private HashMap<UUID, List<ByteArrayWrapper>> hScanSnapShots;
+ private HashMap<UUID, Long> hScanSnapShotCreationTimes;
+ private ScheduledExecutorService HSCANSnapshotExpirationExecutor = null;
+ private int HSCAN_SNAPSHOTS_EXPIRE_MILLISECONDS;
+ private final int TIME_FOR_SNAPSHOTS_TO_LIVE;
+
+
+ public RedisHash(List<ByteArrayWrapper> fieldsToSet, int
hscanSnapShotExpirationInMillis) {
+ this.hash = new HashMap<>();
+ this.hScanSnapShots = new HashMap<>();
+ this.hScanSnapShotCreationTimes = new HashMap<>();
+ this.HSCAN_SNAPSHOTS_EXPIRE_MILLISECONDS = hscanSnapShotExpirationInMillis;
+ this.TIME_FOR_SNAPSHOTS_TO_LIVE = this.HSCAN_SNAPSHOTS_EXPIRE_MILLISECONDS;
- public RedisHash(List<ByteArrayWrapper> fieldsToSet) {
- hash = new HashMap<>();
Iterator<ByteArrayWrapper> iterator = fieldsToSet.iterator();
+
while (iterator.hasNext()) {
hashPut(iterator.next(), iterator.next());
}
}
+ public RedisHash(List<ByteArrayWrapper> fieldsToSet) {
+ this(fieldsToSet, 300000);
+ }
+
+ // for serialization
public RedisHash() {
- // for serialization
+ HSCAN_SNAPSHOTS_EXPIRE_MILLISECONDS = 0;
+ this.TIME_FOR_SNAPSHOTS_TO_LIVE = 0;
+ }
+
+ @VisibleForTesting
+ public HashMap<UUID, List<ByteArrayWrapper>> getHscanSnapShots() {
+ return this.hScanSnapShots;
+ }
+
+ private void expireHScanSnapshots() {
+
+ this.hScanSnapShotCreationTimes.entrySet().forEach(entry -> {
Review comment:
There's a very small window for a `ConcurrentModificationException` to
happen if something else is modifying this collection at the same time. Please
raise a Jira to fix in the future.
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -203,49 +261,120 @@ public int hstrlen(ByteArrayWrapper field) {
return new ArrayList<>(hash.keySet());
}
- public Pair<BigInteger, List<Object>> hscan(Pattern matchPattern, int count,
BigInteger cursor) {
- List<Object> returnList = new ArrayList<Object>();
- int size = hash.size();
- BigInteger beforeCursor = new BigInteger("0");
- int numElements = 0;
- int i = -1;
- for (Map.Entry<ByteArrayWrapper, ByteArrayWrapper> entry :
hash.entrySet()) {
- ByteArrayWrapper key = entry.getKey();
- ByteArrayWrapper value = entry.getValue();
- i++;
- if (beforeCursor.compareTo(cursor) < 0) {
- beforeCursor = beforeCursor.add(new BigInteger("1"));
+ public Pair<BigInteger, List<Object>> hscan(UUID clientID, Pattern
matchPattern,
+ int count,
+ BigInteger cursorParameter) {
+
+ int startCursor = cursorParameter.intValue();
+
+ List<ByteArrayWrapper> keysToScan = getSnapShotOfKeySet(clientID);
+
+ List<Object> resultList =
+ getResultList(keysToScan, startCursor, count, matchPattern);
+
+ int indexOfLast = resultList.size() - 1;
+ int numberOfIterationsCompleted =
Integer.parseInt(resultList.get(indexOfLast).toString());
+ resultList.remove(indexOfLast);
+
+ int returnCursorValueAsInt =
+ getCursorValueToReturn(startCursor, numberOfIterationsCompleted,
keysToScan);
+
+ if (returnCursorValueAsInt == 0) {
+ removeHSCANSnapshot(clientID);
+ }
+ return new ImmutablePair<>(BigInteger.valueOf(returnCursorValueAsInt),
resultList);
+ }
+
+ private void removeHSCANSnapshot(UUID clientID) {
+ this.hScanSnapShots.remove(clientID);
+ this.hScanSnapShotCreationTimes.remove(clientID);
+
+ if (this.hScanSnapShots.isEmpty()) {
+ shutDownHscanSnapshotScheduledRemoval();
+ }
+ }
+
+ private List<Object> getResultList(List<ByteArrayWrapper> keysSnapShot,
+ int startCursor,
+ int count,
+ Pattern matchPattern) {
+
+ int indexOfKeys = 0;
+
+ List<Object> resultList = new ArrayList<>();
+
+ for (ByteArrayWrapper key : keysSnapShot) {
+
+ if (indexOfKeys < startCursor) {
+ indexOfKeys++;
+ continue;
+ }
+
+ indexOfKeys++;
Review comment:
Since `keysSnapShot` is indexable, can this be a simple `for` loop that
starts at `startCursor` and avoid this 'skipping' logic entirely?
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -46,19 +52,71 @@
public class RedisHash extends AbstractRedisData {
public static final RedisHash NULL_REDIS_HASH = new NullRedisHash();
private HashMap<ByteArrayWrapper, ByteArrayWrapper> hash;
+ private HashMap<UUID, List<ByteArrayWrapper>> hScanSnapShots;
+ private HashMap<UUID, Long> hScanSnapShotCreationTimes;
+ private ScheduledExecutorService HSCANSnapshotExpirationExecutor = null;
+ private int HSCAN_SNAPSHOTS_EXPIRE_MILLISECONDS;
+ private final int TIME_FOR_SNAPSHOTS_TO_LIVE;
Review comment:
I think these fields are redundant. This value should also be
configurable by the user somehow. Please could you raise a Jira for that.
##########
File path: geode-redis/README.md
##########
@@ -167,53 +167,53 @@ start server \
| HGETALL | FLUSHALL
| ACL LOG |
| HMGET | FLUSHDB
| ACL SAVE |
| HMSET | GETBIT
| ACL SETUSER |
-| HSET | GETRANGE
| ACL USERS |
-| HSETNX | GETSET
| ACL WHOAMI |
-| HSTRLEN | HDEL
| BGREWRITEAOF |
-| HLEN | HEXISTS
| BGSAVE |
-| HVALS | HGET
| BITFIELD |
-| KEYS | HINCRBY
| BLPOP |
-| PERSIST | HINCRBYFLOAT
| BRPOP |
-| PEXPIRE | HKEYS
| BRPOPLPUSH |
-| PEXPIREAT | HSCAN
| BZPOPMAX |
-| PING | INCR
| BZPOPMIN |
-| PSUBSCRIBE | INCRBY
| CLIENT CACHING |
-| PTTL | INCRBYFLOAT
| CLIENT GETNAME |
-| PUBLISH | INFO
| CLIENT GETREDIR |
-| PUNSUBSCRIBE | MGET
| CLIENT ID |
-| QUIT | MSET
| CLIENT KILL |
-| RENAME | MSETNX
| CLIENT LIST |
-| SADD | PSETEX
| CLIENT PAUSE |
-| SET | SCAN
| CLIENT REPLY |
-| SMEMBERS | SCARD
| CLIENT SETNAME |
-| SREM | SDIFF
| CLIENT TRACKING |
-| SUBSCRIBE | SDIFFSTORE
| CLIENT UNBLOCK |
-| TTL | SELECT
| CLUSTER ADDSLOTS |
-| TYPE | SETBIT
| CLUSTER BUMPEPOCH |
-| UNSUBSCRIBE | SETEX
| CLUSTER COUNT-FAILURE-REPORTS |
-| | SETNX
| CLUSTER COUNTKEYSINSLOT |
-| | SETRANGE
| CLUSTER DELSLOTS |
-| | SHUTDOWN
| CLUSTER FAILOVER |
-| | SINTER
| CLUSTER FLUSHSLOTS |
-| | SINTERSTORE
| CLUSTER FORGET |
-| | SISMEMBER
| CLUSTER GETKEYSINSLOT |
-| | SLOWLOG
| CLUSTER INFO |
-| | SMOVE
| CLUSTER KEYSLOT |
-| | SPOP
| CLUSTER MEET |
-| | SRANDMEMBER
| CLUSTER MYID |
-| | SSCAN
| CLUSTER NODES |
-| | STRLEN
| CLUSTER REPLICAS |
-| | SUNION
| CLUSTER REPLICATE |
-| | SUNIONSTORE
| CLUSTER RESET |
-| | TIME
| CLUSTER SAVECONFIG |
-| | UNLINK [1]
| CLUSTER SET-CONFIG-EPOCH |
-| |
| CLUSTER SETSLOT |
-| |
| CLUSTER SLAVES |
-| |
| CLUSTER SLOTS |
-| |
| COMMAND |
-| |
| COMMAND COUNT |
-| |
| COMMAND GETKEYS |
-| |
| COMMAND INFO |
+| HSCAN | GETRANGE
| ACL USERS |
Review comment:
We were going to make all doc-related changes as one separate commit, so
this can be reverted.
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -203,49 +261,120 @@ public int hstrlen(ByteArrayWrapper field) {
return new ArrayList<>(hash.keySet());
}
- public Pair<BigInteger, List<Object>> hscan(Pattern matchPattern, int count,
BigInteger cursor) {
- List<Object> returnList = new ArrayList<Object>();
- int size = hash.size();
- BigInteger beforeCursor = new BigInteger("0");
- int numElements = 0;
- int i = -1;
- for (Map.Entry<ByteArrayWrapper, ByteArrayWrapper> entry :
hash.entrySet()) {
- ByteArrayWrapper key = entry.getKey();
- ByteArrayWrapper value = entry.getValue();
- i++;
- if (beforeCursor.compareTo(cursor) < 0) {
- beforeCursor = beforeCursor.add(new BigInteger("1"));
+ public Pair<BigInteger, List<Object>> hscan(UUID clientID, Pattern
matchPattern,
+ int count,
+ BigInteger cursorParameter) {
+
+ int startCursor = cursorParameter.intValue();
+
+ List<ByteArrayWrapper> keysToScan = getSnapShotOfKeySet(clientID);
+
+ List<Object> resultList =
+ getResultList(keysToScan, startCursor, count, matchPattern);
+
+ int indexOfLast = resultList.size() - 1;
+ int numberOfIterationsCompleted =
Integer.parseInt(resultList.get(indexOfLast).toString());
+ resultList.remove(indexOfLast);
Review comment:
See below: rather return a `Pair` so that the intent is clearer.
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -203,49 +261,120 @@ public int hstrlen(ByteArrayWrapper field) {
return new ArrayList<>(hash.keySet());
}
- public Pair<BigInteger, List<Object>> hscan(Pattern matchPattern, int count,
BigInteger cursor) {
- List<Object> returnList = new ArrayList<Object>();
- int size = hash.size();
- BigInteger beforeCursor = new BigInteger("0");
- int numElements = 0;
- int i = -1;
- for (Map.Entry<ByteArrayWrapper, ByteArrayWrapper> entry :
hash.entrySet()) {
- ByteArrayWrapper key = entry.getKey();
- ByteArrayWrapper value = entry.getValue();
- i++;
- if (beforeCursor.compareTo(cursor) < 0) {
- beforeCursor = beforeCursor.add(new BigInteger("1"));
+ public Pair<BigInteger, List<Object>> hscan(UUID clientID, Pattern
matchPattern,
+ int count,
+ BigInteger cursorParameter) {
+
+ int startCursor = cursorParameter.intValue();
+
+ List<ByteArrayWrapper> keysToScan = getSnapShotOfKeySet(clientID);
+
+ List<Object> resultList =
+ getResultList(keysToScan, startCursor, count, matchPattern);
+
+ int indexOfLast = resultList.size() - 1;
+ int numberOfIterationsCompleted =
Integer.parseInt(resultList.get(indexOfLast).toString());
+ resultList.remove(indexOfLast);
+
+ int returnCursorValueAsInt =
+ getCursorValueToReturn(startCursor, numberOfIterationsCompleted,
keysToScan);
+
+ if (returnCursorValueAsInt == 0) {
+ removeHSCANSnapshot(clientID);
+ }
+ return new ImmutablePair<>(BigInteger.valueOf(returnCursorValueAsInt),
resultList);
+ }
+
+ private void removeHSCANSnapshot(UUID clientID) {
+ this.hScanSnapShots.remove(clientID);
+ this.hScanSnapShotCreationTimes.remove(clientID);
+
+ if (this.hScanSnapShots.isEmpty()) {
+ shutDownHscanSnapshotScheduledRemoval();
+ }
+ }
+
+ private List<Object> getResultList(List<ByteArrayWrapper> keysSnapShot,
+ int startCursor,
+ int count,
+ Pattern matchPattern) {
+
+ int indexOfKeys = 0;
+
+ List<Object> resultList = new ArrayList<>();
+
+ for (ByteArrayWrapper key : keysSnapShot) {
+
+ if (indexOfKeys < startCursor) {
+ indexOfKeys++;
+ continue;
+ }
+
+ indexOfKeys++;
+
+ ByteArrayWrapper value = hash.get(key);
+ if (value == null) {
continue;
}
if (matchPattern != null) {
if (matchPattern.matcher(key.toString()).matches()) {
- returnList.add(key);
- returnList.add(value);
- numElements++;
+ resultList.add(key);
+ resultList.add(value);
}
} else {
- returnList.add(key);
- returnList.add(value);
- numElements++;
+ resultList.add(key);
+ resultList.add(value);
}
- if (numElements == count) {
+ if ((indexOfKeys - startCursor) == count) {
Review comment:
Should this not instead be checking the size of `resultList` so that
when you've actually accumulated `count` values it will return?
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -203,49 +261,120 @@ public int hstrlen(ByteArrayWrapper field) {
return new ArrayList<>(hash.keySet());
}
- public Pair<BigInteger, List<Object>> hscan(Pattern matchPattern, int count,
BigInteger cursor) {
- List<Object> returnList = new ArrayList<Object>();
- int size = hash.size();
- BigInteger beforeCursor = new BigInteger("0");
- int numElements = 0;
- int i = -1;
- for (Map.Entry<ByteArrayWrapper, ByteArrayWrapper> entry :
hash.entrySet()) {
- ByteArrayWrapper key = entry.getKey();
- ByteArrayWrapper value = entry.getValue();
- i++;
- if (beforeCursor.compareTo(cursor) < 0) {
- beforeCursor = beforeCursor.add(new BigInteger("1"));
+ public Pair<BigInteger, List<Object>> hscan(UUID clientID, Pattern
matchPattern,
+ int count,
+ BigInteger cursorParameter) {
+
+ int startCursor = cursorParameter.intValue();
+
+ List<ByteArrayWrapper> keysToScan = getSnapShotOfKeySet(clientID);
+
+ List<Object> resultList =
+ getResultList(keysToScan, startCursor, count, matchPattern);
+
+ int indexOfLast = resultList.size() - 1;
+ int numberOfIterationsCompleted =
Integer.parseInt(resultList.get(indexOfLast).toString());
+ resultList.remove(indexOfLast);
+
+ int returnCursorValueAsInt =
+ getCursorValueToReturn(startCursor, numberOfIterationsCompleted,
keysToScan);
+
+ if (returnCursorValueAsInt == 0) {
+ removeHSCANSnapshot(clientID);
+ }
+ return new ImmutablePair<>(BigInteger.valueOf(returnCursorValueAsInt),
resultList);
+ }
+
+ private void removeHSCANSnapshot(UUID clientID) {
+ this.hScanSnapShots.remove(clientID);
+ this.hScanSnapShotCreationTimes.remove(clientID);
+
+ if (this.hScanSnapShots.isEmpty()) {
+ shutDownHscanSnapshotScheduledRemoval();
+ }
+ }
+
+ private List<Object> getResultList(List<ByteArrayWrapper> keysSnapShot,
+ int startCursor,
+ int count,
+ Pattern matchPattern) {
+
+ int indexOfKeys = 0;
+
+ List<Object> resultList = new ArrayList<>();
+
+ for (ByteArrayWrapper key : keysSnapShot) {
+
+ if (indexOfKeys < startCursor) {
+ indexOfKeys++;
+ continue;
+ }
+
+ indexOfKeys++;
+
+ ByteArrayWrapper value = hash.get(key);
+ if (value == null) {
continue;
}
if (matchPattern != null) {
if (matchPattern.matcher(key.toString()).matches()) {
- returnList.add(key);
- returnList.add(value);
- numElements++;
+ resultList.add(key);
+ resultList.add(value);
}
} else {
- returnList.add(key);
- returnList.add(value);
- numElements++;
+ resultList.add(key);
+ resultList.add(value);
}
- if (numElements == count) {
+ if ((indexOfKeys - startCursor) == count) {
break;
}
+
+ }
+ resultList.add(new ByteArrayWrapper(String.valueOf(indexOfKeys -
startCursor).getBytes()));
Review comment:
If you do need to return 2 distinct values, please use a `Pair` instead
to make it clearer.
----------------------------------------------------------------
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:
[email protected]
