jdeppe-pivotal commented on a change in pull request #6616:
URL: https://github.com/apache/geode/pull/6616#discussion_r654871509
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -41,24 +44,32 @@
import org.apache.geode.redis.internal.netty.Coder;
import org.apache.geode.redis.mocks.MockSubscriber;
import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
public abstract class AbstractSubCommandsIntegrationTest implements
RedisIntegrationTest {
+
+ @ClassRule
+ public static ExecutorServiceRule executor = new ExecutorServiceRule();
+
private Jedis subscriber;
private Jedis introspector;
private MockSubscriber mockSubscriber;
@Before
public void setup() {
mockSubscriber = new MockSubscriber();
- subscriber = new Jedis("localhost", getPort());
- introspector = new Jedis("localhost", getPort());
+ subscriber = new Jedis(BIND_ADDRESS, getPort(), REDIS_CLIENT_TIMEOUT);
+ introspector = new Jedis(BIND_ADDRESS, getPort(), REDIS_CLIENT_TIMEOUT);
}
@After
public void teardown() {
if (mockSubscriber.getSubscribedChannels() > 0) {
mockSubscriber.unsubscribe();
}
+ if (mockSubscriber.getSubscribedChannels() > 0) {
+ mockSubscriber.punsubscribe();
+ }
Review comment:
There is a race condition here. In the jedis code, if the subscribers
end up going to 0 then there is a possiblilty of a NPE for subsequent
(p)unsubscribe calls if we don't first do the check.
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -191,6 +201,67 @@ public void
channels_shouldNotReturnDuplicates_givenMultipleSubscribersToSameCha
subscriber2.close();
}
+ /** -- NUMSUB-- **/
+
+ @Test
+ public void numsub_shouldReturnEmptyList_whenCalledWithOutChannelNames() {
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1);
+
+ List<Object> result =
+ uncheckedCast(introspector.sendCommand(Protocol.Command.PUBSUB,
PUBSUB_NUMSUB));
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void
numsub_shouldReturnListOfChannelsWithSubscriberCount_whenCalledWithActiveChannels()
{
+ Jedis subscriber2 = new Jedis(BIND_ADDRESS, getPort(),
REDIS_CLIENT_TIMEOUT);
+ MockSubscriber fooAndBarSubscriber = new MockSubscriber();
+
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ executor.submit(() -> subscriber2.subscribe(fooAndBarSubscriber, "foo",
"bar"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1
+ && fooAndBarSubscriber.getSubscribedChannels() == 2);
+
+ HashMap<String, String> result =
Review comment:
Done
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -191,6 +201,67 @@ public void
channels_shouldNotReturnDuplicates_givenMultipleSubscribersToSameCha
subscriber2.close();
}
+ /** -- NUMSUB-- **/
+
+ @Test
+ public void numsub_shouldReturnEmptyList_whenCalledWithOutChannelNames() {
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1);
+
+ List<Object> result =
+ uncheckedCast(introspector.sendCommand(Protocol.Command.PUBSUB,
PUBSUB_NUMSUB));
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void
numsub_shouldReturnListOfChannelsWithSubscriberCount_whenCalledWithActiveChannels()
{
+ Jedis subscriber2 = new Jedis(BIND_ADDRESS, getPort(),
REDIS_CLIENT_TIMEOUT);
+ MockSubscriber fooAndBarSubscriber = new MockSubscriber();
+
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ executor.submit(() -> subscriber2.subscribe(fooAndBarSubscriber, "foo",
"bar"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1
+ && fooAndBarSubscriber.getSubscribedChannels() == 2);
+
+ HashMap<String, String> result =
+ (HashMap<String, String>) introspector.pubsubNumSub("foo", "bar");
+
+ assertThat(result.containsKey("foo")).isTrue();
+ assertThat(result.containsKey("bar")).isTrue();
+ assertThat(result.get("foo")).isEqualTo("2");
+ assertThat(result.get("bar")).isEqualTo("1");
+
+ fooAndBarSubscriber.unsubscribe();
+
+ waitFor(() -> fooAndBarSubscriber.getSubscribedChannels() == 0);
+ subscriber2.close();
+ }
+
+ @Test
+ public void
numsub_shouldReturnChannelNameWithZero_whenCalledWithChannelWithNoSubscribers()
{
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1);
+
+ HashMap<String, String> result =
Review comment:
Done
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -191,6 +201,67 @@ public void
channels_shouldNotReturnDuplicates_givenMultipleSubscribersToSameCha
subscriber2.close();
}
+ /** -- NUMSUB-- **/
+
+ @Test
+ public void numsub_shouldReturnEmptyList_whenCalledWithOutChannelNames() {
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1);
+
+ List<Object> result =
+ uncheckedCast(introspector.sendCommand(Protocol.Command.PUBSUB,
PUBSUB_NUMSUB));
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void
numsub_shouldReturnListOfChannelsWithSubscriberCount_whenCalledWithActiveChannels()
{
+ Jedis subscriber2 = new Jedis(BIND_ADDRESS, getPort(),
REDIS_CLIENT_TIMEOUT);
+ MockSubscriber fooAndBarSubscriber = new MockSubscriber();
+
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ executor.submit(() -> subscriber2.subscribe(fooAndBarSubscriber, "foo",
"bar"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1
+ && fooAndBarSubscriber.getSubscribedChannels() == 2);
+
+ HashMap<String, String> result =
+ (HashMap<String, String>) introspector.pubsubNumSub("foo", "bar");
+
+ assertThat(result.containsKey("foo")).isTrue();
+ assertThat(result.containsKey("bar")).isTrue();
+ assertThat(result.get("foo")).isEqualTo("2");
+ assertThat(result.get("bar")).isEqualTo("1");
+
+ fooAndBarSubscriber.unsubscribe();
+
+ waitFor(() -> fooAndBarSubscriber.getSubscribedChannels() == 0);
+ subscriber2.close();
+ }
+
+ @Test
+ public void
numsub_shouldReturnChannelNameWithZero_whenCalledWithChannelWithNoSubscribers()
{
+ executor.submit(() -> subscriber.subscribe(mockSubscriber, "foo"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1);
+
+ HashMap<String, String> result =
+ (HashMap<String, String>) introspector.pubsubNumSub("bar");
+
+ assertThat(result.containsKey("bar")).isTrue();
+ assertThat(result.get("bar")).isEqualTo("0");
+ }
+
+ @Test
+ public void
numsub_shouldReturnPatternWithZero_whenCalledWithPatternWithNoChannelSubscribers()
{
+ executor.submit(() -> subscriber.psubscribe(mockSubscriber, "f*"));
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 1);
+
+ HashMap<String, String> result = (HashMap<String, String>)
introspector.pubsubNumSub("f*");
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.
For queries about this service, please contact Infrastructure at:
[email protected]
