You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2019/12/23 03:32:05 UTC

[GitHub] [pulsar] wolfstudy opened a new pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

wolfstudy opened a new pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915
 
 
   Signed-off-by: xiaolong.ran <rx...@apache.org>
   
   Master Issue: #4080 
   
   ### Modifications
   
   - add `set-delayed-delivery` policy for namespace

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


With regards,
Apache Git Services

[GitHub] [pulsar] tuteng commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
tuteng commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-601985084
 
 
   Add label release-2.5.1, due #6183 dependency

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-576002099
 
 
   Sorry, i missing the message.
   
   @codelipenghui fixed done, please take a look again, thanks.

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


With regards,
Apache Git Services

[GitHub] [pulsar] sijie commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
sijie commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-570011885
 
 
   @codelipenghui can you help review this pull request again?

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


With regards,
Apache Git Services

[GitHub] [pulsar] codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#discussion_r363265639
 
 

 ##########
 File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
 ##########
 @@ -730,6 +730,7 @@ public boolean trackDelayedDelivery(long ledgerId, long entryId, MessageMetadata
                         .of(topic.getBrokerService().getDelayedDeliveryTrackerFactory().newTracker(this));
             }
 
+            delayedDeliveryTracker.get().resetTickTime(topic.checkTickTimeDelayedDelivery());
 
 Review comment:
   Same as above comment, reset tick time when the namespace policy changed is more economic than check need to reset for every message

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy edited a comment on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy edited a comment on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-569062982
 
 
   @codelipenghui PTAL again, thanks.
   
   ### Get
   
   ```
   $ ./bin/pulsar-admin namespaces get-delayed-delivery public/default
   
   {
     "tickTime" : 1000,
     "active" : true
   }
   ```
   
   ### Set
   
   ```
   $ ./bin/pulsar-admin namespaces set-delayed-delivery public/default --enable --time 2s
   ```
   

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy removed a comment on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy removed a comment on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-568456978
 
 
   @codelipenghui @sijie @merlimat @jiazhai PTAL

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


With regards,
Apache Git Services

[GitHub] [pulsar] sijie commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
sijie commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-570989776
 
 
   @codelipenghui can you review this change?

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-569062982
 
 
   @codelipenghui PTAL again, thanks.
   ```
   $ ./bin/pulsar-admin namespaces get-delayed-delivery public/default
   
   {
     "tickTime" : 1000,
     "active" : true
   }
   ```

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-576670419
 
 
   run java8 tests
   run cpp 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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-576123315
 
 
   retest this please

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


With regards,
Apache Git Services

[GitHub] [pulsar] congbobo184 commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#discussion_r362728240
 
 

 ##########
 File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java
 ##########
 @@ -1152,7 +1153,69 @@ void clearNamespaceBundleBacklogForSubscription(String namespace, String bundle,
      */
     void setEncryptionRequiredStatus(String namespace, boolean encryptionRequired) throws PulsarAdminException;
 
-     /**
+    /**
+     * Get the delayed delivery messages for all topics within a namespace.
+     * <p>
+     * If disabled, messages will be immediately delivered and there will
+     * be no tracking overhead.
+     * <p>
+     * Request example:
+     *
+     * <pre>
+     * <code>
+     * {
+     *     "tickTime" : 1000, // Enable or disable delayed delivery for messages on a namespace
+     *     "active" : true,   // The tick time for when retrying on delayed delivery messages
+     * }
+     * </code>
+     * </pre>
+     *
+     * @param namespace
+     *            Namespace name
+     * @return delayedDeliveryPolicies
+     *            Whether to enable the delayed delivery for messages.
+     *
+     * @throws NotAuthorizedException
+     *             Don't have admin permission
+     * @throws NotFoundException
+     *             Namespace does not exist
+     * @throws PulsarAdminException
+     *             Unexpected error
+     */
+    DelayedDeliveryPolicies getDelayedDelivery(String namespace) throws PulsarAdminException;
+
+    /**
+     * Set the delayed delivery messages for all topics within a namespace.
+     * <p>
+     * If disabled, messages will be immediately delivered and there will
+     * be no tracking overhead.
+     * <p>
+     * Request example:
+     *
+     * <pre>
+     * <code>
+     * {
+     *     "tickTime" : 1000, // Enable or disable delayed delivery for messages on a namespace
+     *     "active" : true,   // The tick time for when retrying on delayed delivery messages
 
 Review comment:
   are the two notes reversed?

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy merged pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915
 
 
   

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


With regards,
Apache Git Services

[GitHub] [pulsar] codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#discussion_r361113675
 
 

 ##########
 File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/Policies.java
 ##########
 @@ -64,6 +64,10 @@
     @SuppressWarnings("checkstyle:MemberName")
     public boolean encryption_required = false;
     @SuppressWarnings("checkstyle:MemberName")
+    public boolean delayed_delivery = true;
+    @SuppressWarnings("checkstyle:MemberName")
+    public long delayed_delivery_time = 0;
 
 Review comment:
   It should be delayed_delivery_tick_time

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-576687709
 
 
   run java8 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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-568456978
 
 
   @codelipenghui @sijie @merlimat @jiazhai PTAL

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


With regards,
Apache Git Services

[GitHub] [pulsar] codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#discussion_r368353184
 
 

 ##########
 File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
 ##########
 @@ -718,7 +718,7 @@ public void initializeDispatchRateLimiterIfNeeded(Optional<Policies> policies) {
 
     @Override
     public boolean trackDelayedDelivery(long ledgerId, long entryId, MessageMetadata msgMetadata) {
-        if (!isDelayedDeliveryEnabled) {
+        if (!isDelayedDeliveryEnabled || !topic.delayedDeliveryEnabled) {
 
 Review comment:
   I think you can remove the isDelayedDeliveryEnabled in dispatcher, maybe we just need it on topic level on subscription level

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy edited a comment on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy edited a comment on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-576002099
 
 
   Sorry, i missed the message.
   
   @codelipenghui fixed done, please take a look again, thanks.

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


With regards,
Apache Git Services

[GitHub] [pulsar] codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#discussion_r368353491
 
 

 ##########
 File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
 ##########
 @@ -299,6 +299,8 @@ public BrokerService(PulsarService pulsar) throws Exception {
         this.delayedDeliveryTrackerFactory = DelayedDeliveryTrackerLoader
                 .loadDelayedDeliveryTrackerFactory(pulsar.getConfiguration());
 
+
+
 
 Review comment:
   remove these 2 lines

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


With regards,
Apache Git Services

[GitHub] [pulsar] codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#discussion_r363264482
 
 

 ##########
 File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
 ##########
 @@ -718,7 +718,7 @@ public void initializeDispatchRateLimiterIfNeeded(Optional<Policies> policies) {
 
     @Override
     public boolean trackDelayedDelivery(long ledgerId, long entryId, MessageMetadata msgMetadata) {
-        if (!isDelayedDeliveryEnabled) {
+        if (!isDelayedDeliveryEnabled || !topic.checkActiveDelayedDelivery()) {
 
 Review comment:
   The isDelayedDeliveryEnabled in the dispatcher is for indicating the dispatcher is enable or disable delayed delivery. It's better to update this field while the delayed delivery policy changed because it's more economic than get policy from a hashmap for each message. 

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


With regards,
Apache Git Services

[GitHub] [pulsar] wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #5915: Allow to enable/disable delayed delivery for messages on namespace
URL: https://github.com/apache/pulsar/pull/5915#issuecomment-569258838
 
 
   ping @codelipenghui PTAL again, thanks.

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


With regards,
Apache Git Services