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 2022/08/29 01:40:23 UTC
[GitHub] [pulsar] lin-zhao opened a new pull request, #17320: [fix][doc] Add more information for producer_request_hold policy
lin-zhao opened a new pull request, #17320:
URL: https://github.com/apache/pulsar/pull/17320
Clarifies that producer_request_hold holds the message for at most client operation timeout.
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
### Motivation
Clarifies the behavior of producer_request_hold policy that raised some confusion in the community.
### Modifications
- Namespaces admin doc includes mention of client operation timeout when explaining producer_request_hold
- Cookbook for retention expiry includes mention of client operation timeout in the example for producer_request_hold
### Documentation
- [x ] `doc`
(Your 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] Technoboy- commented on pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#issuecomment-1231785055
> https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L1759-L1762
yes, right.
--
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] lin-zhao commented on a diff in pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
lin-zhao commented on code in PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#discussion_r963071278
##########
site2/docs/admin-api-namespaces.md:
##########
@@ -288,9 +288,9 @@ admin.namespaces().getNamespaceReplicationClusters(namespace)
Backlog quota helps the broker to restrict bandwidth/storage of a namespace once it reaches a certain threshold limit. Admin can set the limit and take corresponding action after the limit is reached.
- 1. producer_request_hold: broker holds but not persists produce request payload
+ 1. producer_request_hold: the producer holds the message and retries until client configuration `operationTimeoutMs` is exceeded
Review Comment:
Addressed. Good catch. `operationTimeoutMs` applies for producer creation but `sendTimeoutMs` applies for send requests.
##########
site2/docs/admin-api-namespaces.md:
##########
@@ -288,9 +288,9 @@ admin.namespaces().getNamespaceReplicationClusters(namespace)
Backlog quota helps the broker to restrict bandwidth/storage of a namespace once it reaches a certain threshold limit. Admin can set the limit and take corresponding action after the limit is reached.
- 1. producer_request_hold: broker holds but not persists produce request payload
+ 1. producer_request_hold: the producer holds the message and retries until client configuration `operationTimeoutMs` is exceeded
Review Comment:
Addressed.
--
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] Technoboy- commented on pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#issuecomment-1231794456
> @Technoboy- Could you please review this PR from the technical perspective? If it is technically correct, then technical writers can review it from the technical writing perspective. Thank you! 😊
LGTM
--
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] lin-zhao commented on pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
lin-zhao commented on PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#issuecomment-1232241073
Please review 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.
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 pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#issuecomment-1229645558
@Technoboy-
Could you please review this PR from the technical perspective?
If it is technically correct, then technical writers can review it from the technical writing perspective.
Thank you! 😊
--
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] lin-zhao commented on pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
lin-zhao commented on PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#issuecomment-1232235444
> According to the `BacklogQuotaManager`'s logic here: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BacklogQuotaManager.java#L104-L107 I think the behavior should be broker disconnect producer when it detect backlog quota exceeded, and producer will get `ProducerBlockedQuotaExceededError` which is retryable, so it'll keep reconnecting till backlog is cleared: https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L1759-L1762 So no data will be send to broker in such base and producer is hold the data till backlog is cleared. Will let Jiwei confirm.
You are exactly right. This is the right logic for this policy. Though I think the documentation for the policy should be really concise without all the details behind the scene.
--
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] lin-zhao commented on a diff in pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
lin-zhao commented on code in PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#discussion_r958985104
##########
site2/docs/admin-api-namespaces.md:
##########
@@ -288,7 +288,7 @@ admin.namespaces().getNamespaceReplicationClusters(namespace)
Backlog quota helps the broker to restrict bandwidth/storage of a namespace once it reaches a certain threshold limit. Admin can set the limit and take corresponding action after the limit is reached.
- 1. producer_request_hold: broker holds but not persists produce request payload
+ 1. producer_request_hold: broker holds but not persists produce request payload until client configuration `operationTimeoutMs` is exceeded.
Review Comment:
@merlimat you are exactly right that broker actually does **not** hold the message. I will make an update.
--
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] codelipenghui commented on a diff in pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#discussion_r959161657
##########
site2/docs/admin-api-namespaces.md:
##########
@@ -288,9 +288,9 @@ admin.namespaces().getNamespaceReplicationClusters(namespace)
Backlog quota helps the broker to restrict bandwidth/storage of a namespace once it reaches a certain threshold limit. Admin can set the limit and take corresponding action after the limit is reached.
- 1. producer_request_hold: broker holds but not persists produce request payload
+ 1. producer_request_hold: the producer holds the message and retries until client configuration `operationTimeoutMs` is exceeded
Review Comment:
It should be until client configuration `sendTimeoutMs` is exceeded.
The `operationTimeoutMs` is for producer creation or consumer creation.
--
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] MarvinCai commented on pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#issuecomment-1230421582
According to the `BacklogQuotaManager`'s logic here: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BacklogQuotaManager.java#L104-L107
I think the behavior should be broker disconnect producer when it detect backlog quota exceeded, and producer will get `ProducerBlockedQuotaExceededError` which is retryable, so it'll keep reconnecting till backlog is cleared: https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L1759-L1762
So no data will be send to broker in such base and producer is hold the data till backlog is cleared.
Will let Jiwei confirm.
--
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] merlimat commented on a diff in pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #17320:
URL: https://github.com/apache/pulsar/pull/17320#discussion_r958635465
##########
site2/docs/admin-api-namespaces.md:
##########
@@ -288,7 +288,7 @@ admin.namespaces().getNamespaceReplicationClusters(namespace)
Backlog quota helps the broker to restrict bandwidth/storage of a namespace once it reaches a certain threshold limit. Admin can set the limit and take corresponding action after the limit is reached.
- 1. producer_request_hold: broker holds but not persists produce request payload
+ 1. producer_request_hold: broker holds but not persists produce request payload until client configuration `operationTimeoutMs` is exceeded.
Review Comment:
This is not exactly correct. First, the broker does not hold the payload: it's the client that will keep retrying up to the `sendTimeout` configured in the producer.
--
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] codelipenghui merged pull request #17320: [fix][doc] Add more information for producer_request_hold policy
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17320:
URL: https://github.com/apache/pulsar/pull/17320
--
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