[GitHub] [geode] upthewaterspout commented on a change in pull request #6704: GEODE-9338: Remove strong guarantees for Radish PUBLISH command

[GitBox <gi...@apache.org>]

upthewaterspout commented on a change in pull request #6704:
URL: https://github.com/apache/geode/pull/6704#discussion_r676848022



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/services/StripedExecutorService.java
##########
@@ -0,0 +1,518 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license

Review comment:
       Since this file came from a third party, I think you need to leave their license in place at the top of the file - https://www.apache.org/legal/src-headers.html#3party

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSubImpl.java
##########
@@ -42,13 +47,45 @@
 public class PubSubImpl implements PubSub {
   public static final String REDIS_PUB_SUB_FUNCTION_ID = "redisPubSubFunctionID";
 
+  private static final int DEFAULT_PUBLISH_THREAD_COUNT =
+      Integer.getInteger("redis.publish-thread-count", 100);

Review comment:
       Huh, that's a lot of threads. I wonder if we really should default to this many. I guess there is no way to have this StripedExecutorService thing adapt to changing load?

##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubDUnitTest.java
##########
@@ -209,20 +210,18 @@ public void shouldContinueToFunction_whenOneServerShutsDownGracefully_givenTwoSu
     MockSubscriber mockSubscriber1 = new MockSubscriber(latch);
     MockSubscriber mockSubscriber2 = new MockSubscriber(latch);
 
-    Future<Void> subscriber1Future =
-        executor.submit(() -> subscriber1.subscribe(mockSubscriber1, CHANNEL_NAME));
+    executor.submit(() -> subscriber1.subscribe(mockSubscriber1, CHANNEL_NAME));
     Future<Void> subscriber2Future =
         executor.submit(() -> subscriber2.subscribe(mockSubscriber2, CHANNEL_NAME));
 
     assertThat(latch.await(30, TimeUnit.SECONDS)).as("channel subscription was not received")
         .isTrue();
 
-    Long result = publisher1.publish(CHANNEL_NAME, "hello");
-    assertThat(result).isEqualTo(2);
+    publisher1.publish(CHANNEL_NAME, "hello");

Review comment:
       We changed the meaning and behavior of the results of the publish command - now it is only returning the number of subscribers on the current node. Does that mean we no longer know what to assert here? Are there other tests that are checking that what we return from publish makes sense?




-- 
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