sabbey37 commented on a change in pull request #6493:
URL: https://github.com/apache/geode/pull/6493#discussion_r635242890
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/pubsub/SubscriptionsJUnitTest.java
##########
@@ -195,4 +197,70 @@ public void removeByClientAndPattern() {
channelSubscriberOne,
channelSubscriberTwo);
}
+
+ @Test
+ public void
findChannelNames_shouldReturnAllChannelNames_whenCalledWithoutParameter() {
+ Subscriptions subject = new Subscriptions();
+
+ Channel channel = mock(Channel.class);
+ when(channel.closeFuture()).thenReturn(mock(ChannelFuture.class));
+ Client client = new Client(channel);
+ ExecutionHandlerContext context = mock(ExecutionHandlerContext.class);
+
+ Subscription subscriptionFoo =
+ new ChannelSubscription(client, "foo".getBytes(), context, subject);
+
+ Subscription subscriptionBar =
+ new ChannelSubscription(client, "bar".getBytes(), context, subject);
+
+ subject.add(subscriptionFoo);
+ subject.add(subscriptionBar);
+
+ List<byte[]> result = subject.findChannelNames();
+
+ assertThat(result).containsExactlyInAnyOrder("foo".getBytes(),
"bar".getBytes());
+ }
+
+ @Test
+ public void
findChannelNames_shouldReturnOnlyMatchingChannelNames_whenCalledWithPattern() {
+ Subscriptions subject = new Subscriptions();
+ byte[] pattern = "b*".getBytes();
+
+ Channel channel = mock(Channel.class);
+ when(channel.closeFuture()).thenReturn(mock(ChannelFuture.class));
+ Client client = new Client(channel);
+ ExecutionHandlerContext context = mock(ExecutionHandlerContext.class);
+
+ subject.add(new ChannelSubscription(client, "foo".getBytes(), context,
subject));
+ subject.add(new ChannelSubscription(client, "bar".getBytes(), context,
subject));
+ subject.add(new ChannelSubscription(client, "barbarella".getBytes(),
context, subject));
Review comment:
What about `BarbaraAnn`? 😜
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/pubsub/SubCommandExecutor.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ *
+ */
+
+
+package org.apache.geode.redis.internal.executor.pubsub;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.apache.geode.redis.internal.executor.Executor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class SubCommandExecutor implements Executor {
Review comment:
Since there are a lot of commands in Redis that have sub commands, I
think it would make more sense to follow the pattern of our other executors and
name this after the primary command, so `PubsubExecutor` (or `PubSubExecutor`?)
(also, all related test classes would need to be renamed). Check out
`ClusterExecutor`, which is named after the primary command, but has a case
switch for sub commands.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/pubsub/SubCommandExecutor.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ *
+ */
+
+
+package org.apache.geode.redis.internal.executor.pubsub;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.apache.geode.redis.internal.executor.Executor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class SubCommandExecutor implements Executor {
+
+ static final String UNKNOWN_SUBCOMMAND_ERROR_BASE =
+ "Unknown subcommand or wrong number of arguments for '%s'";
Review comment:
I'm guessing we'll add the `Try PUBSUB HELP.` message to the end of this
once `PUBSUB HELP` is implemented? I think we should put this message in the
`RedisConstants` file where other similar error messages live and rename it to
follow the same pattern as the others, maybe `ERROR_UNKNOWN_PUBSUB_SUBCOMMAND`
since it will eventually be specific to PUBSUB.... we could actually refactor
those subcommand error messages to all use the same base and insert the
relevant primary command, so something like:
```
ERROR_UNKNOWN_SUBCOMMAND =
"Unknown subcommand or wrong number of arguments for '%1$s'. Try %2$s
HELP."
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -0,0 +1,156 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ *
+ */
+
+package org.apache.geode.redis.internal.executor.pubsub;
+
+import static
org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static
org.apache.geode.redis.internal.executor.pubsub.SubCommandExecutor.UNKNOWN_SUBCOMMAND_ERROR_BASE;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.ArrayList;
+import java.util.concurrent.Callable;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.mocks.MockSubscriber;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+
+public abstract class AbstractSubCommandsIntegrationTest implements
RedisIntegrationTest {
+ 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());
+ }
+
+ @After
+ public void teardown() {
+ if (mockSubscriber.getSubscribedChannels() > 0) {
+ mockSubscriber.unsubscribe();
+ }
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 0);
+ }
+
+ @Test
+ public void Pubsub_shouldError_givenTooFewArguments() {
+ assertAtLeastNArgs(introspector, Protocol.Command.PUBSUB, 1);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void pubsub_shouldReturnError_givenUnknownSubcommand() {
+ String expected = String.format(UNKNOWN_SUBCOMMAND_ERROR_BASE, "nonesuch");
+
+ assertThatThrownBy(() -> introspector.sendCommand(Protocol.Command.PUBSUB,
"nonesuch"))
+ .hasMessageContaining(expected);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void
channels_shouldReturnListOfAllChannels_whenCalledWithoutPattern() {
+ ArrayList<byte[]> expectedChannels = new ArrayList<>();
+ expectedChannels.add("foo".getBytes());
+ expectedChannels.add("bar".getBytes());
+ Runnable runnable =
+ () -> subscriber.subscribe(mockSubscriber, "foo", "bar");
+ Thread subscriberThread = new Thread(runnable);
+
+ subscriberThread.start();
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 2);
+ ArrayList<byte[]> result = (ArrayList<byte[]>) introspector
+ .sendCommand(Protocol.Command.PUBSUB, "channels");
+
+ assertThat(result).containsExactlyInAnyOrderElementsOf(expectedChannels);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void
channels_shouldReturnListOfMatchingChannels_whenCalledWithPattern() {
+ ArrayList<byte[]> expectedChannels = new ArrayList<>();
+ expectedChannels.add("foo".getBytes());
+ Runnable runnable =
+ () -> subscriber.subscribe(mockSubscriber, "foo", "bar");
+ Thread subscriberThread = new Thread(runnable);
+
+ subscriberThread.start();
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 2);
+ ArrayList<byte[]> result =
+ (ArrayList<byte[]>) introspector
+ .sendCommand(Protocol.Command.PUBSUB, "channels", "fo*");
Review comment:
For this test as well as the test below it we could use
`introspector.pubsubChannels("fo*")`
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -0,0 +1,156 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ *
+ */
+
+package org.apache.geode.redis.internal.executor.pubsub;
+
+import static
org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static
org.apache.geode.redis.internal.executor.pubsub.SubCommandExecutor.UNKNOWN_SUBCOMMAND_ERROR_BASE;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.ArrayList;
+import java.util.concurrent.Callable;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.mocks.MockSubscriber;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+
+public abstract class AbstractSubCommandsIntegrationTest implements
RedisIntegrationTest {
+ 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());
+ }
+
+ @After
+ public void teardown() {
+ if (mockSubscriber.getSubscribedChannels() > 0) {
+ mockSubscriber.unsubscribe();
+ }
+ waitFor(() -> mockSubscriber.getSubscribedChannels() == 0);
+ }
+
+ @Test
+ public void Pubsub_shouldError_givenTooFewArguments() {
+ assertAtLeastNArgs(introspector, Protocol.Command.PUBSUB, 1);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void pubsub_shouldReturnError_givenUnknownSubcommand() {
+ String expected = String.format(UNKNOWN_SUBCOMMAND_ERROR_BASE, "nonesuch");
+
+ assertThatThrownBy(() -> introspector.sendCommand(Protocol.Command.PUBSUB,
"nonesuch"))
+ .hasMessageContaining(expected);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void
channels_shouldReturnListOfAllChannels_whenCalledWithoutPattern() {
+ ArrayList<byte[]> expectedChannels = new ArrayList<>();
+ expectedChannels.add("foo".getBytes());
+ expectedChannels.add("bar".getBytes());
+ Runnable runnable =
+ () -> subscriber.subscribe(mockSubscriber, "foo", "bar");
+ Thread subscriberThread = new Thread(runnable);
+
+ subscriberThread.start();
Review comment:
In some of our other tests with subscriberThreads, we have a finally
block that cleans up by unsubscribing and waiting for the thread to no longer
be alive. Do we need to do that in these tests?
--
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:
us...@infra.apache.org
