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 2021/09/08 18:21:44 UTC
[GitHub] [pulsar] zwalsh-toast opened a new pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
zwalsh-toast opened a new pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974
Fixes #11846
### Motivation
Control the message flow when creating a consumer. Before this change, consumers would immediately send flow permits to the broker upon `.subscribe()`. Now, consumers can connect (which validates configuration/authentication) immediately, but wait to process messages until `.resume()` is called.
### Modifications
* Add a field `private boolean startPaused` to `ConsumerConfigurationData`
* Use that boolean to set `paused` in the `ConsumerImpl`
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change added tests and can be verified as follows:
- *Added a unit test that confirms the paused state of the consumer based on the ConsumerConfigurationData*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (yes)
- There is a new option `paused(boolean)` on the `ConsumerBuilder`
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [ ] doc-required
(If you need help on updating docs, create a doc issue)
- [x] no-need-doc
This PR adds Javadoc to the new ConsumerBuilder option. There's no existing documentation I can find on the site for `paused` so this seems sufficient.
- [ ] doc
(If this PR contains doc changes)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall merged pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#discussion_r704854674
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -741,4 +741,12 @@
* corruption, deserialization error, etc.).
*/
ConsumerBuilder<T> poolMessages(boolean poolMessages);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer will not immediately fetch messages when
Review comment:
```suggestion
* Start the consumer in a paused state. When enabled, the consumer does not immediately fetch messages when
```
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -741,4 +741,12 @@
* corruption, deserialization error, etc.).
*/
ConsumerBuilder<T> poolMessages(boolean poolMessages);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer will not immediately fetch messages when
+ * {@link #subscribe()} is called. Instead, the consumer will wait to fetch messages until {@link Consumer#resume()} is called.
Review comment:
```suggestion
* {@link #subscribe()} is called. Instead, the consumer waits to fetch messages until {@link Consumer#resume()} is called.
```
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -741,4 +741,12 @@
* corruption, deserialization error, etc.).
*/
ConsumerBuilder<T> poolMessages(boolean poolMessages);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer will not immediately fetch messages when
Review comment:
Use present tense in tech writing.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zwalsh-toast commented on pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
zwalsh-toast commented on pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#issuecomment-969325272
Bump on this PR! @eolivelli, @codelipenghui, @BewareMyPower, @sijie, @hangc0276, @merlimat - 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#discussion_r766226285
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/ConsumerImplTest.java
##########
@@ -200,4 +201,21 @@ public void testClose() {
}
Assert.assertNull(checkException);
}
+
+ @Test
+ public void testConsumerCreatedWhilePaused() throws InterruptedException {
Review comment:
I think it makes sense to keep both 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#issuecomment-918564979
@eolivelli, @codelipenghui, @BewareMyPower, @sijie, @hangc0276, @merlimat - PTAL, 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zwalsh-toast commented on a change in pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
zwalsh-toast commented on a change in pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#discussion_r766222152
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/ConsumerImplTest.java
##########
@@ -200,4 +201,21 @@ public void testClose() {
}
Assert.assertNull(checkException);
}
+
+ @Test
+ public void testConsumerCreatedWhilePaused() throws InterruptedException {
Review comment:
I added a test in the `ConsumerBuilderImplTest` class 👍🏻
If you meant I should update _this_ test to use the builder - I can do that too! (though it's a little less natural with the way this test class is currently set up)
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -763,4 +763,12 @@
* </pre>
*/
ConsumerBuilder<T> negativeAckRedeliveryBackoff(NegativeAckRedeliveryBackoff negativeAckRedeliveryBackoff);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer does not immediately fetch messages when
Review comment:
good call, done
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -763,4 +763,12 @@
* </pre>
*/
ConsumerBuilder<T> negativeAckRedeliveryBackoff(NegativeAckRedeliveryBackoff negativeAckRedeliveryBackoff);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer does not immediately fetch messages when
+ * {@link #subscribe()} is called. Instead, the consumer waits to fetch messages until {@link Consumer#resume()} is called.
+ * <p/>
+ * See also {@link Consumer#pause()}.
+ */
+ ConsumerBuilder<T> paused(boolean paused);
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zwalsh-toast commented on pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
zwalsh-toast commented on pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#issuecomment-915468971
Hi @codelipenghui, I think this change should address the issue #11846 we discussed - could you review?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#issuecomment-969680844
LGTM. But I'm wondering if it needs a PIP because it's a new feature for Pulsar client while it's very simple.
Please help confirm it. @merlimat @codelipenghui
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #11974: [Issue 11846][Java client] Create a consumer in the paused state
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11974:
URL: https://github.com/apache/pulsar/pull/11974#discussion_r761432632
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -763,4 +763,12 @@
* </pre>
*/
ConsumerBuilder<T> negativeAckRedeliveryBackoff(NegativeAckRedeliveryBackoff negativeAckRedeliveryBackoff);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer does not immediately fetch messages when
+ * {@link #subscribe()} is called. Instead, the consumer waits to fetch messages until {@link Consumer#resume()} is called.
+ * <p/>
+ * See also {@link Consumer#pause()}.
+ */
+ ConsumerBuilder<T> paused(boolean paused);
Review comment:
@zwalsh-toast - what do you think about renaming this method to `startPaused`? It would then align with the configuration class.
```suggestion
ConsumerBuilder<T> startPaused(boolean startPaused);
```
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/ConsumerImplTest.java
##########
@@ -200,4 +201,21 @@ public void testClose() {
}
Assert.assertNull(checkException);
}
+
+ @Test
+ public void testConsumerCreatedWhilePaused() throws InterruptedException {
Review comment:
Do you think you could write a test that relies on the `ConsumerBuilder` implementation? That way we can exercise all of your changes.
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java
##########
@@ -763,4 +763,12 @@
* </pre>
*/
ConsumerBuilder<T> negativeAckRedeliveryBackoff(NegativeAckRedeliveryBackoff negativeAckRedeliveryBackoff);
+
+ /**
+ * Start the consumer in a paused state. When enabled, the consumer does not immediately fetch messages when
Review comment:
I think it would be helpful to state in the Javadoc that the default value for `paused` (or `startPaused`) is `false`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org