You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/07/15 23:16:05 UTC
[GitHub] [geode] DonalEvans opened a new pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
DonalEvans opened a new pull request #6701:
URL: https://github.com/apache/geode/pull/6701
This PR is intentionally split into two commits; first an initial test clean-up and refactor, and then the commit containing the changes related to the ticket.
- Refactor and clean up AbstractHScanIntegrationTest
- Refactor RedisHash.hscan() to handle large COUNT values
- Add tests to cover new behaviour
Authored-by: Donal Evans <do...@vmware.com>
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 build` run 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](http://www.apache.org/legal/resolved.html#category-a)?
### 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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r674961831
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
+ LogService.getLogger().error(
+ "The size of the data to be returned by hscan, {}, exceeds the maximum capacity of an array",
+ maximumCapacity);
+ throw new OutOfMemoryError("Requested array size exceeds VM limit");
Review comment:
How about ERROR_NOT_INTEGER? In this case the size is "out of range". Also be sure not to use OutOfMemoryError.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r674408022
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
+ LogService.getLogger().error(
+ "The size of the data to be returned by hscan, {}, exceeds the maximum capacity of an array",
+ maximumCapacity);
+ throw new OutOfMemoryError("Requested array size exceeds VM limit");
Review comment:
Currently, the exception thrown here will ultimately get handled in `ExecutionHandlerContext.getExceptionResponse()` where it will result in an error message returned to the client with the message "The server had an internal error please try again". This definitely doesn't seem correct, since trying again will just result in the same error, but none of the currently existing error responses we have seem right either, and I'm having a hard time find a list of existing Redis error messages to see if any of them would be a better fit. It's also not practical to test what Redis actually does in this situation, given that would require a Redis hash with over a billion entries. I'll have a look at the Redis source and see if I can figure out what the behaviour would be in this situation.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r672429805
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Yeah, I think I tend to agree with you on this one. I suspect that the vast majority of users would rather their cache be faster than that it be able to handle an improbably large number of keys/values. I'll rework this part of the code and see about maybe introducing some error logging if we hit that unlikely scenario.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r674408800
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
+ LogService.getLogger().error(
Review comment:
I've changed the log level to info and made the message more descriptive/helpful.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r674220656
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
+ LogService.getLogger().error(
+ "The size of the data to be returned by hscan, {}, exceeds the maximum capacity of an array",
+ maximumCapacity);
+ throw new OutOfMemoryError("Requested array size exceeds VM limit");
Review comment:
I think OutOfMemoryError should be reserved for when you actually run out of heap. In this case you are preventing that from happening by not doing a large allocation. Some code will no longer trust the state of the jvm if it sees an OutOfMemoryError and will initiate a shutdown. You could use IllegalArgumentException instead. Whatever exception is thrown here should it be caught in HScanExecutor and converted to RedisResponse.error?
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
Review comment:
change hash.size() to hlen()
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
+ LogService.getLogger().error(
Review comment:
I'm not sure what we should do about logging in these cases. Normally we do not log something in a server when a client does something wrong. We just have the server respond to the client with an error message. I know in some cases we are limited by the redis standard error responses in giving the user extra information about what they did wrong. In that case we could add some info in the server log but maybe it would be at the "info" log level. The "error" level is usually reserved for when something unexpected happened on the server but it thinks it can keep running. The "warning" level is if things don't look healthy (but it might be okay) and we want to get the admin's attention. The "fatal" level is if something is wrong and the server needs to shutdown.
In this case I would not log anything here if we can tell the client what was wrong. If we can't get a response back to the client then I think this should be "info" and describe what the client did wrong.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
Review comment:
Where you able to test this in the integration test? I know you did in the unit test but it is hard to do when you need the hash to have more than 1G entries. If you could use the same hlen() trick in the integration test that you used in the unit test then you could test how this exception will be handled by the higher levels of the code.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r670877174
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
Review comment:
shouldn't this be "< initialCapacity" instead of "< count"?
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
Review comment:
should initialCapacity be named maximumCapacity?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r671472287
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Conceptually I like pairing the key and value here, but it is an additional memory allocation that doesn't add any functional value AFAICT.
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHScanIntegrationTest.java
##########
@@ -577,16 +569,44 @@ private void multipleHScanAndAssertOnSizeOfResultSet(Jedis jedis,
.isEqualTo(initialHashData.size());
}
- private Map<String, String> makeEntrySet(int sizeOfDataSet) {
+ private Map<String, String> initializeThreeFieldHash() {
+ Map<String, String> entryMap = new HashMap<>();
+ entryMap.put(FIELD_ONE, VALUE_ONE);
+ entryMap.put(FIELD_TWO, VALUE_TWO);
+ entryMap.put(FIELD_THREE, VALUE_THREE);
+ jedis.hmset(HASH_KEY, entryMap);
+ return entryMap;
+ }
+
+ Map<byte[], byte[]> initializeThreeFieldHashBytes() {
+ Map<byte[], byte[]> entryMap = new HashMap<>();
+ entryMap.put(FIELD_ONE.getBytes(), VALUE_ONE.getBytes());
+ entryMap.put(FIELD_TWO.getBytes(), VALUE_TWO.getBytes());
+ entryMap.put(FIELD_THREE_BYTES, VALUE_THREE.getBytes());
+ jedis.hmset(HASH_KEY.getBytes(), entryMap);
+ return entryMap;
+ }
+
+ private Map<String, String> makeEntryMap() {
Map<String, String> dataSet = new HashMap<>();
- for (int i = 0; i < sizeOfDataSet; i++) {
+ for (int i = 0; i < SIZE_OF_ENTRY_MAP; i++) {
dataSet.put(BASE_FIELD + i, "value_" + i);
}
return dataSet;
}
- private int getSlotForKey(String key) {
- int crc = CRC16.calculate(key);
+ private int getSlotForKey() {
Review comment:
The slot could just be a static field now.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r674382114
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
Review comment:
Nice catch, thanks
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r672392625
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Ah, OK I see. Out of curiosity I did some back-of-the-napkin tests and it seems like we'd need a VM with at least 80-100GB of heap to even hold enough data to hit this max (in the 'incorrect' implementation). And twice that for the 'correct' implementation. I'm really torn, but I do feel that it's more pragmatic to err on the side of performance than cater to a use case that seems highly unlikely (famous last words :) ). To think about it a different way, we'd always end up paying the cost of allocation just to avoid the incredibly unlikely error situation. (For which there is a solution - just request fewer than 2 billion keys at a time).
Not to overly complicate this, but technically it shouldn't be difficult to optimise for the most frequent use-case but have a special case for when the requested count does exceed `Integer.MAX_VALUE / 2`. I'm not suggesting you do that now though.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans merged pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #6701:
URL: https://github.com/apache/geode/pull/6701
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r672392625
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Ah, OK I see. Out of curiosity I did some back-of-the-napkin tests and it seems like we'd need a VM with at least 80-100GB of heap to even hold enough data to hit this max (in the 'incorrect' implementation). And twice that for the 'correct' implementation. I'm really torn, but I do feel that it's more pragmatic to err on the side of performance than cater to a use case that seems highly unlikely (famous last words :) ). To think about it a different way, we'd always end up paying the cost of allocation just to avoid the incredibly unlikely error situation. (For which there is a solution - just request fewer than 2 billion keys at a time).
Not to overly complicate this, but technically it shouldn't be difficult to optimise for the most frequent use-case but have a special case for when the requested count does exceed `Integer.MAX_VALUE / 2`. I'm not suggesting you do that now though.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r674390885
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
Review comment:
The integration tests are designed to simulate the behaviour of a client connecting to a Redis cluster, and to be agnostic about whether that cluster is a Geode Radish cluster or a native Redis cluster, so it's not really possible to get under the hood and mock out the returns of methods in our classes to force certain behaviours, unfortunately.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r672429805
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Yeah, I think I tend to agree with you on this one. I suspect that the vast majority of users would rather their cache be faster than that it be able to handle an improbably large number of keys/values. I'll rework this part of the code and see about maybe introducing some error logging if we hit that unlikely scenario.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r672429805
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Yeah, I think I tend to agree with you on this one. I suspect that the vast majority of users would rather their cache be faster than that it be able to handle an improbably large number of keys/values. I'll rework this part of the code and see about maybe introducing some error logging if we hit that unlikely scenario.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r671483756
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
It's necessary to wrap the key and value in some object here, since the maximum length of a list returned by `HSCAN` is 2*(number of keys) which could in theory be greater than `Integer.MAX_VALUE`. If we want to assume that a user will never have that many keys in their Redish hash, then we can dispense with this pairing of values to increase efficiency and document that the maximum number of Redis hash keys we support is `Integer.MAX_VALUE / 2`, but if we want to be safe, then this is necessary.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r672392625
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+ int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash
+ int initialCapacity = Math.min(count, hash.size());
+ List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
do {
cursor = hash.scan(cursor, 1,
(list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
- } while (cursor != 0 && resultList.size() < (count * 2));
+ } while (cursor != 0 && resultList.size() < count);
return new ImmutablePair<>(cursor, resultList);
}
- private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
- byte[] value) {
+ private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+ byte[] key, byte[] value) {
if (matchPattern != null) {
if (matchPattern.matcher(bytesToString(key)).matches()) {
- resultList.add(key);
- resultList.add(value);
+ resultList.add(new ImmutablePair<>(key, value));
Review comment:
Ah, OK I see. Out of curiosity I did some back-of-the-napkin tests and it seems like we'd need a VM with at least 80-100GB of heap to even hold enough data to hit this max (in the 'incorrect' implementation). And twice that for the 'correct' implementation. I'm really torn, but I do feel that it's more pragmatic to err on the side of performance than cater to a use case that seems highly unlikely (famous last words :) ). To think about it a different way, we'd always end up paying the cost of allocation just to avoid the incredibly unlikely error situation. (For which there is a solution - just request fewer than 2 billion keys at a time).
Not to overly complicate this, but technically it shouldn't be difficult to optimise for the most frequent use-case but have a special case for when the requested count does exceed `Integer.MAX_VALUE / 2`. I'm not suggesting you do that now though.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6701:
URL: https://github.com/apache/geode/pull/6701#discussion_r675061450
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,15 +238,21 @@ public int hstrlen(byte[] field) {
return new ArrayList<>(hash.keySet());
}
- public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
- int count,
- int cursor) {
-
- ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+ public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern, int count, int cursor) {
+ // No need to allocate more space than it's possible to use given the size of the hash. We need
+ // to add 1 to hash.size() to ensure that if count > hash.size(), we return a cursor of 0
+ long maximumCapacity = 2L * Math.min(count, hlen() + 1);
+ if (maximumCapacity > Integer.MAX_VALUE) {
+ LogService.getLogger().error(
+ "The size of the data to be returned by hscan, {}, exceeds the maximum capacity of an array",
+ maximumCapacity);
+ throw new OutOfMemoryError("Requested array size exceeds VM limit");
Review comment:
Sounds good to me. I checked Redis' code and they don't hit this issue, since they're able to allocate lists with a maximum length of Long.MAX_VALUE, which is greater than their maximum number of entries in a hash, so this is one case where we're not able to exactly match their behaviour. Thankfully, it's also a case that should never realistically come up, since it's highly unlikely that users will be putting over a billion entries into one Redis hash.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org