[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6704: GEODE-9338: remove publish strong guarantees

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

dschneider-pivotal commented on a change in pull request #6704:
URL: https://github.com/apache/geode/pull/6704#discussion_r674993888



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSubImpl.java
##########
@@ -58,23 +62,25 @@ public int getSubscriptionCount() {
 
   @Override
   public long publish(RegionProvider regionProvider, byte[] channel, byte[] message) {
-    Set<DistributedMember> membersWithDataRegion = regionProvider.getRegionMembers();
-    @SuppressWarnings("unchecked")
-    ResultCollector<String[], List<Long>> subscriberCountCollector = FunctionService
-        .onMembers(membersWithDataRegion)
-        .setArguments(new Object[] {channel, message})
-        .execute(REDIS_PUB_SUB_FUNCTION_ID);
+    executor.submit(() -> internalPublish(regionProvider, channel, message));
 
-    List<Long> subscriberCounts = null;
+    return subscriptions.findSubscriptions(channel).size();

Review comment:
       consider calling findSubscriptions(channel).size() first and saving its value in "long result".
   Then only call executor.submit if result > 0.
   Then return result.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSub.java
##########
@@ -31,10 +31,8 @@
   /**
    * Publish a message on a channel
    *
-   *
    * @param channel to publish to
    * @param message to publish
-   * @return the number of messages published
    */
   long publish(RegionProvider regionProvider, byte[] channel, byte[] message);

Review comment:
       since publish still returns a long it seems like you should still have a comment describing what is returned. How about "return an estimate of the number of messages published by this call". It might also be worth enhancing this comment to say that this call may return before the actual publish has been done. Something about it being async.




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