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/10/20 16:07:12 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
jdeppe-pivotal opened a new pull request #7019:
URL: https://github.com/apache/geode/pull/7019
<!-- 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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r737949790
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscriptionManager.java
##########
@@ -87,6 +91,16 @@ public int getSubscriptionCount() {
return sum;
}
+ public int getUniqueSubscriptionCount() {
Review comment:
Add unit test coverage for this method to PatternSubscriptionManagerTest
--
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 merged pull request #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #7019:
URL: https://github.com/apache/geode/pull/7019
--
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r737947110
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/executor/pubsub/PubSubExecutor.java
##########
@@ -54,7 +54,7 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse
.error(String.format(ERROR_UNKNOWN_PUBSUB_SUBCOMMAND, new String(subCommand)));
}
- long numPatResponse = context.getPubSub().findNumberOfSubscribedPatterns();
+ long numPatResponse = context.getPubSub().findNumberOfUniqueSubscribedPatterns();
Review comment:
this seems like a bug you found in our PUBSUB command impl. Did it only show up with 6.2?
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscriptionManager.java
##########
@@ -87,6 +91,16 @@ public int getSubscriptionCount() {
return sum;
}
+ public int getUniqueSubscriptionCount() {
+ Set<byte[]> uniques = new ObjectOpenCustomHashSet<>(ByteArrays.HASH_STRATEGY);
Review comment:
I think this is overkill. On this class "clientManagers" is already a map of unique channel/patterns to ClientSubscriptionManager instances. So all you need to do in this method is return "clientManagers.size()".
--
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r737806788
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/internal/proxy/RedisProxyInboundHandler.java
##########
@@ -129,6 +133,13 @@ public void channelRead(final ChannelHandlerContext ctx, Object msg) {
outboundHandler.addResponseProcessor(nodesResponseProcessor);
}
break;
+ case "hello":
+ // Lettuce tries to use RESP 3 if possible but Netty's Redis codecs can't handle that yet.
+ // We implicitly fake the client into thinking this is an older Redis so that it uses
+ // RESP 2.
+ RedisMessage error = new ErrorRedisMessage("ERR unknown command");
Review comment:
Lettuce just cares that an error is returned. There doesn't seem to be any benefit in adding more detail to the error response at this point.
--
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r737805868
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscriptionManager.java
##########
@@ -87,6 +90,18 @@ public int getSubscriptionCount() {
return sum;
}
+ public int getUniqueSubscriptionCount() {
+ Set<ByteBuffer> uniques = new HashSet<>();
+ for (ClientSubscriptionManager manager : clientManagers.values()) {
+ for (Subscription s : manager.getSubscriptions()) {
+ for (byte[] bytes : s.getClient().getPatternSubscriptions()) {
+ uniques.add(ByteBuffer.wrap(bytes).asReadOnlyBuffer());
+ }
+ }
+ }
+ return uniques.size();
Review comment:
Ah - nice!
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/AbstractUnknownIntegrationTest.java
##########
@@ -64,12 +64,6 @@ public void givenUnknownCommand_withEmptyStringArgument_returnsUnknownCommandErr
"ERR unknown command `fhqwhgads`, with args beginning with: `EVERYBODY`, ``, ");
}
- @Test // HELLO is not a recognized command until Redis 6.0.0
- public void givenHelloCommand_returnsUnknownCommandErrorWithArgumentsListed() {
- assertThatThrownBy(() -> jedis.sendCommand(() -> "HELLO".getBytes()))
- .hasMessage("ERR unknown command `HELLO`, with args beginning with: ");
- }
-
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.
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r738654883
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscriptionManager.java
##########
@@ -87,6 +91,16 @@ public int getSubscriptionCount() {
return sum;
}
+ public int getUniqueSubscriptionCount() {
Review comment:
This is now gone in favor of just calling `size()`.
--
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r737484601
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscriptionManager.java
##########
@@ -87,6 +90,18 @@ public int getSubscriptionCount() {
return sum;
}
+ public int getUniqueSubscriptionCount() {
+ Set<ByteBuffer> uniques = new HashSet<>();
+ for (ClientSubscriptionManager manager : clientManagers.values()) {
+ for (Subscription s : manager.getSubscriptions()) {
+ for (byte[] bytes : s.getClient().getPatternSubscriptions()) {
+ uniques.add(ByteBuffer.wrap(bytes).asReadOnlyBuffer());
+ }
+ }
+ }
+ return uniques.size();
Review comment:
I don't know if it's necessarily better, but this can be simplified somewhat by using an `ObjectOpenCustomHashSet`:
```
Set<byte[]> uniques = new ObjectOpenCustomHashSet<>(ByteArrays.HASH_STRATEGY);
for (ClientSubscriptionManager manager : clientManagers.values()) {
for (Subscription s : manager.getSubscriptions()) {
uniques.addAll(s.getClient().getPatternSubscriptions());
}
}
return uniques.size();
```
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/AbstractUnknownIntegrationTest.java
##########
@@ -64,12 +64,6 @@ public void givenUnknownCommand_withEmptyStringArgument_returnsUnknownCommandErr
"ERR unknown command `fhqwhgads`, with args beginning with: `EVERYBODY`, ``, ");
}
- @Test // HELLO is not a recognized command until Redis 6.0.0
- public void givenHelloCommand_returnsUnknownCommandErrorWithArgumentsListed() {
- assertThatThrownBy(() -> jedis.sendCommand(() -> "HELLO".getBytes()))
- .hasMessage("ERR unknown command `HELLO`, with args beginning with: ");
- }
-
Review comment:
Could a modified version of this test be moved to `UnknownIntegrationTest`? Removing it entirely loses us coverage of this behaviour, I think.
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/internal/proxy/RedisProxyInboundHandler.java
##########
@@ -129,6 +133,13 @@ public void channelRead(final ChannelHandlerContext ctx, Object msg) {
outboundHandler.addResponseProcessor(nodesResponseProcessor);
}
break;
+ case "hello":
+ // Lettuce tries to use RESP 3 if possible but Netty's Redis codecs can't handle that yet.
+ // We implicitly fake the client into thinking this is an older Redis so that it uses
+ // RESP 2.
+ RedisMessage error = new ErrorRedisMessage("ERR unknown command");
Review comment:
Should this error message follow the same format as other unknown command messages, namely
```
String.format(ERROR_UNKNOWN_COMMAND, commandText, commandArguments)
```
--
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r738654191
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/executor/pubsub/PubSubExecutor.java
##########
@@ -54,7 +54,7 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse
.error(String.format(ERROR_UNKNOWN_PUBSUB_SUBCOMMAND, new String(subCommand)));
}
- long numPatResponse = context.getPubSub().findNumberOfSubscribedPatterns();
+ long numPatResponse = context.getPubSub().findNumberOfUniqueSubscribedPatterns();
Review comment:
Yes it did. It seems like it is correct as per the documentation for `PUBSUB NUMPAT`.
--
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 #7019: GEODE-9646: Update native redis tests to consistently use version 6.2.6
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7019:
URL: https://github.com/apache/geode/pull/7019#discussion_r738654618
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscriptionManager.java
##########
@@ -87,6 +91,16 @@ public int getSubscriptionCount() {
return sum;
}
+ public int getUniqueSubscriptionCount() {
+ Set<byte[]> uniques = new ObjectOpenCustomHashSet<>(ByteArrays.HASH_STRATEGY);
Review comment:
Good point. I've added a `size()` method to `AbstractSubscriptionManager`.
--
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