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 2020/10/19 05:26:20 UTC
[GitHub] [geode] sabbey37 opened a new pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
sabbey37 opened a new pull request #5639:
URL: https://github.com/apache/geode/pull/5639
Geode Redis should unsubscribe from all channels (not including patterns) when no channel is provided and punsubscribe from all patterns (not including channels) when no pattern is provided.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r509288415
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSub.java
##########
@@ -81,19 +81,19 @@ long publish(
long punsubscribe(GlobPattern pattern, Client client);
/**
- * Return a list of channel names that a client has subscribed to
+ * Return a list of channel names or patterns that a client has subscribed to
*
* @param client the Client which is to be queried
- * @return the list of channels
+ * @return the list of channels or patterns
*/
- List<byte[]> findSubscribedChannels(Client client);
+ List<byte[]> findSubscriptionsNames(Client client, Subscription.Type type);
/**
- * Return a list of pattern names that a client has subscribed to
+ * Return a list of channel names and patterns that a client has subscribed to
*
* @param client the Client which is to be queried
- * @return the list of patterns
+ * @return the list of channels and patterns
*/
- List<byte[]> findSubscribedPatterns(Client client);
+ List<byte[]> findSubscriptionsNames(Client client);;
Review comment:
Yes! Thank you for pointing this out!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 merged pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 merged pull request #5639:
URL: https://github.com/apache/geode/pull/5639
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507884986
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubscriptionsIntegrationTest.java
##########
@@ -88,18 +111,91 @@ public void multiSubscribe() {
.untilAsserted(() -> assertThat(mockSubscriber.getReceivedPMessages()).hasSize(1));
assertThat(mockSubscriber.getReceivedEvents()).containsExactly("message", "pmessage");
mockSubscriber.unsubscribe();
- client.close();
}
@Test
- public void unallowedCommandsWhileSubscribed() {
- Jedis client = new Jedis("localhost", getPort());
+ public void unsubscribingImplicitlyFromAllChannels_doesNotUnsubscribeFromPatterns() {
Review comment:
We do have a test that only subscribes to channels and then unsubscribes without an argument. We did not have a test that subscribes to channels and psubscribes to patterns, then unsubscribes without an argument (or punsubscribes without an argument). We were incorrectly unsubscribing from everything (channels and patterns) when punsubscribe or unsubscribe were called without an argument. Native Redis will only unsubscribe from channels (not patterns) if unsubscribe is called without an argument or punsubscribe only from patterns (not channels) if punsubscribe is called without an argument.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507793073
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubscriptionsIntegrationTest.java
##########
@@ -88,18 +111,91 @@ public void multiSubscribe() {
.untilAsserted(() -> assertThat(mockSubscriber.getReceivedPMessages()).hasSize(1));
assertThat(mockSubscriber.getReceivedEvents()).containsExactly("message", "pmessage");
mockSubscriber.unsubscribe();
- client.close();
}
@Test
- public void unallowedCommandsWhileSubscribed() {
- Jedis client = new Jedis("localhost", getPort());
+ public void unsubscribingImplicitlyFromAllChannels_doesNotUnsubscribeFromPatterns() {
Review comment:
Do we not already have similar tests in `AbstractPubSubIntegrationTest`? Perhaps `testUnsubscribingImplicitlyFromAllChannels` and `testPunsubscribingImplicitlyFromAllChannels`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r508522928
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscription.java
##########
@@ -53,11 +53,15 @@ void publishMessage(byte[] channel, byte[] message,
List<Object> createResponse(byte[] channel, byte[] message);
/**
- * Return the subscription name. In the case of a pattern the string representation of the
- * pattern is returned.
+ * Return the channel name. In the case of a pattern null is returned.
*/
byte[] getChannelName();
+ /**
+ * Return the pattern name. In the case of a channel null is returned.
+ */
+ byte[] getPatternName();
Review comment:
Good idea, I just pushed a change. Let me know what you think!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507886058
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscription.java
##########
@@ -53,11 +53,15 @@ void publishMessage(byte[] channel, byte[] message,
List<Object> createResponse(byte[] channel, byte[] message);
/**
- * Return the subscription name. In the case of a pattern the string representation of the
- * pattern is returned.
+ * Return the channel name. In the case of a pattern null is returned.
*/
byte[] getChannelName();
+ /**
+ * Return the pattern name. In the case of a channel null is returned.
+ */
+ byte[] getPatternName();
Review comment:
I'm definitely open to suggestions on this, maybe we could have another method that's called `getChannelOrPattern()` or some other way of distinguishing whether a channel/pattern name was returned?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r508840773
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSub.java
##########
@@ -81,19 +81,19 @@ long publish(
long punsubscribe(GlobPattern pattern, Client client);
/**
- * Return a list of channel names that a client has subscribed to
+ * Return a list of channel names or patterns that a client has subscribed to
*
* @param client the Client which is to be queried
- * @return the list of channels
+ * @return the list of channels or patterns
*/
- List<byte[]> findSubscribedChannels(Client client);
+ List<byte[]> findSubscriptionsNames(Client client, Subscription.Type type);
/**
- * Return a list of pattern names that a client has subscribed to
+ * Return a list of channel names and patterns that a client has subscribed to
*
* @param client the Client which is to be queried
- * @return the list of patterns
+ * @return the list of channels and patterns
*/
- List<byte[]> findSubscribedPatterns(Client client);
+ List<byte[]> findSubscriptionsNames(Client client);;
Review comment:
Only if you need to push another change: I think this should be `findSubscriptionNames` and the double `;` at the end of the line. No biggy.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSubImpl.java
##########
@@ -140,18 +139,17 @@ public long punsubscribe(GlobPattern pattern, Client client) {
}
@Override
- public List<byte[]> findSubscribedChannels(Client client) {
+ public List<byte[]> findSubscriptionsNames(Client client) {
return subscriptions.findSubscriptions(client).stream()
- .map(Subscription::getChannelName)
- .filter(Objects::nonNull)
+ .map(Subscription::getSubscriptionName)
.collect(Collectors.toList());
}
@Override
- public List<byte[]> findSubscribedPatterns(Client client) {
+ public List<byte[]> findSubscriptionsNames(Client client, Subscription.Type type) {
return subscriptions.findSubscriptions(client).stream()
- .map(Subscription::getPatternName)
- .filter(Objects::nonNull)
+ .filter(s -> s.getType().equals(type))
Review comment:
Also no biggy, but for enum equality it's slightly better to use `==` since it's also null safe.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507977581
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscription.java
##########
@@ -53,11 +53,15 @@ void publishMessage(byte[] channel, byte[] message,
List<Object> createResponse(byte[] channel, byte[] message);
/**
- * Return the subscription name. In the case of a pattern the string representation of the
- * pattern is returned.
+ * Return the channel name. In the case of a pattern null is returned.
*/
byte[] getChannelName();
+ /**
+ * Return the pattern name. In the case of a channel null is returned.
+ */
+ byte[] getPatternName();
Review comment:
I see what the problem is now. I think it would be better to provide a `Type` enum into the `Subscription` interface:
```
enum Type {
CHANNEL,
PATTERN;
}
Type getType();
```
And then you should be able to filter by that when you need to have methods distinguish the type of subscription.
In the `PubSub` interface you can add:
```
List<byte[]> findSubscribedChannels(Client client, Subscription.Type type);
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r509288983
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSubImpl.java
##########
@@ -140,18 +139,17 @@ public long punsubscribe(GlobPattern pattern, Client client) {
}
@Override
- public List<byte[]> findSubscribedChannels(Client client) {
+ public List<byte[]> findSubscriptionsNames(Client client) {
return subscriptions.findSubscriptions(client).stream()
- .map(Subscription::getChannelName)
- .filter(Objects::nonNull)
+ .map(Subscription::getSubscriptionName)
.collect(Collectors.toList());
}
@Override
- public List<byte[]> findSubscribedPatterns(Client client) {
+ public List<byte[]> findSubscriptionsNames(Client client, Subscription.Type type) {
return subscriptions.findSubscriptions(client).stream()
- .map(Subscription::getPatternName)
- .filter(Objects::nonNull)
+ .filter(s -> s.getType().equals(type))
Review comment:
Good to know! I'll update it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507886058
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscription.java
##########
@@ -53,11 +53,15 @@ void publishMessage(byte[] channel, byte[] message,
List<Object> createResponse(byte[] channel, byte[] message);
/**
- * Return the subscription name. In the case of a pattern the string representation of the
- * pattern is returned.
+ * Return the channel name. In the case of a pattern null is returned.
*/
byte[] getChannelName();
+ /**
+ * Return the pattern name. In the case of a channel null is returned.
+ */
+ byte[] getPatternName();
Review comment:
I'm definitely open to suggestions on this. The main reason this method was created and the `getChannelName()` method was changed was to unsubscribe from only channels or only patterns as appropriate. We could always distinguish a channel from a pattern in other ways. What do you think is best?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r508841928
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSubImpl.java
##########
@@ -140,18 +139,17 @@ public long punsubscribe(GlobPattern pattern, Client client) {
}
@Override
- public List<byte[]> findSubscribedChannels(Client client) {
+ public List<byte[]> findSubscriptionsNames(Client client) {
return subscriptions.findSubscriptions(client).stream()
- .map(Subscription::getChannelName)
- .filter(Objects::nonNull)
+ .map(Subscription::getSubscriptionName)
.collect(Collectors.toList());
}
@Override
- public List<byte[]> findSubscribedPatterns(Client client) {
+ public List<byte[]> findSubscriptionsNames(Client client, Subscription.Type type) {
return subscriptions.findSubscriptions(client).stream()
- .map(Subscription::getPatternName)
- .filter(Objects::nonNull)
+ .filter(s -> s.getType().equals(type))
Review comment:
Also no biggy, but for enum equality it's slightly better to use `==` since it's also null safe. Under the covers the JDK ends up doing `==` in any case.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507474516
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubscriptionsIntegrationTest.java
##########
@@ -88,18 +111,91 @@ public void multiSubscribe() {
.untilAsserted(() -> assertThat(mockSubscriber.getReceivedPMessages()).hasSize(1));
assertThat(mockSubscriber.getReceivedEvents()).containsExactly("message", "pmessage");
mockSubscriber.unsubscribe();
- client.close();
}
@Test
- public void unallowedCommandsWhileSubscribed() {
- Jedis client = new Jedis("localhost", getPort());
+ public void unsubscribingImplicitlyFromAllChannels_doesNotUnsubscribeFromPatterns() {
Review comment:
It might make sense to move these test to `AbstractPubSubIntegrationTest.java` once [8577](https://issues.apache.org/jira/projects/GEODE/issues/GEODE-8577?filter=allopenissues) is resolved.
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubscriptionsIntegrationTest.java
##########
@@ -88,18 +111,91 @@ public void multiSubscribe() {
.untilAsserted(() -> assertThat(mockSubscriber.getReceivedPMessages()).hasSize(1));
assertThat(mockSubscriber.getReceivedEvents()).containsExactly("message", "pmessage");
mockSubscriber.unsubscribe();
- client.close();
}
@Test
- public void unallowedCommandsWhileSubscribed() {
- Jedis client = new Jedis("localhost", getPort());
+ public void unsubscribingImplicitlyFromAllChannels_doesNotUnsubscribeFromPatterns() {
Review comment:
It might make sense to move these tests to `AbstractPubSubIntegrationTest.java` once [8577](https://issues.apache.org/jira/projects/GEODE/issues/GEODE-8577?filter=allopenissues) is resolved.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5639: GEODE-8627: Redis not unsubscribing and punsubscribing correctly when no channel/pattern provided
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5639:
URL: https://github.com/apache/geode/pull/5639#discussion_r507788349
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscription.java
##########
@@ -53,11 +53,15 @@ void publishMessage(byte[] channel, byte[] message,
List<Object> createResponse(byte[] channel, byte[] message);
/**
- * Return the subscription name. In the case of a pattern the string representation of the
- * pattern is returned.
+ * Return the channel name. In the case of a pattern null is returned.
*/
byte[] getChannelName();
+ /**
+ * Return the pattern name. In the case of a channel null is returned.
+ */
+ byte[] getPatternName();
Review comment:
I can see that you want the method name to be more expressive, but I don't think it's good practice to have an API where some methods return null depending on what type of object you're using. That opens the door for a dev to easily make a mistake. I'd prefer to keep a single method but give it a more descriptive name. Perhaps `getSubscriptionName()` or `getChannelOrPattern()`.
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubscriptionsIntegrationTest.java
##########
@@ -88,18 +111,91 @@ public void multiSubscribe() {
.untilAsserted(() -> assertThat(mockSubscriber.getReceivedPMessages()).hasSize(1));
assertThat(mockSubscriber.getReceivedEvents()).containsExactly("message", "pmessage");
mockSubscriber.unsubscribe();
- client.close();
}
@Test
- public void unallowedCommandsWhileSubscribed() {
- Jedis client = new Jedis("localhost", getPort());
+ public void unsubscribingImplicitlyFromAllChannels_doesNotUnsubscribeFromPatterns() {
Review comment:
Do we not already have similar tests in `AbstractPubSubIntegrationTest`? Perhaps `testUnsubscribingImplicitlyFromAllChannels` and `testPunsubscribingImplicitlyFromAllChannels`.
----------------------------------------------------------------
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:
users@infra.apache.org