Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[websocket] Query parameter "negativeAckRedeliveryDelay" should be effective even if DLQ is disabled #11495

Merged
merged 2 commits into from Aug 4, 2021

Conversation

massakam
Copy link
Contributor

@massakam massakam commented Jul 29, 2021

Motivation

On the consumer endpoint of WebSocket API, we can specify the delay time before a message which is negatively acknowledged is redelivered using the query parameter negativeAckRedeliveryDelay.

However, this parameter is currently ignored when DLQ is disabled. I think this is an implementation mistake. Users should be able to specify negativeAckRedeliveryDelay even if DLQ is disabled.

if (queryParams.containsKey("maxRedeliverCount") || queryParams.containsKey("deadLetterTopic")) {
DeadLetterPolicy.DeadLetterPolicyBuilder dlpBuilder = DeadLetterPolicy.builder();
if (queryParams.containsKey("maxRedeliverCount")) {
dlpBuilder.maxRedeliverCount(Integer.parseInt(queryParams.get("maxRedeliverCount")))
.deadLetterTopic(String.format("%s-%s-DLQ", topic, subscription));
}
if (queryParams.containsKey("deadLetterTopic")) {
dlpBuilder.deadLetterTopic(queryParams.get("deadLetterTopic"));
}
if (queryParams.containsKey("negativeAckRedeliveryDelay")) {
builder.negativeAckRedeliveryDelay(Integer.parseInt(queryParams.get("negativeAckRedeliveryDelay")), TimeUnit.MILLISECONDS);
}
builder.deadLetterPolicy(dlpBuilder.build());
}

Related PR: #8249

Modifications

Fixed ConsumerHandler of WebSocket to use the negativeAckRedeliveryDelay value specified by the client even if DLQ is disabled. In addition, fixed an inappropriate test code (ProxyPublishConsumeTest#nackMessageTest()).

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (yes)
  • 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)

@massakam massakam added type/bug The PR fixed a bug or issue reported a bug doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. area/websocket labels Jul 29, 2021
@massakam massakam added this to the 2.9.0 milestone Jul 29, 2021
@massakam massakam self-assigned this Jul 29, 2021
site2/docs/client-libraries-websocket.md Outdated Show resolved Hide resolved
Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I’ve left some comments, PTAL.

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
@massakam
Copy link
Contributor Author

massakam commented Aug 2, 2021

@Anonymitaet Addressed your comment. PTAL

Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Approved from the language perspective.

@sijie sijie merged commit 1f76d0d into apache:master Aug 4, 2021
@massakam massakam deleted the fix-ws-nack branch August 4, 2021 05:30
codelipenghui pushed a commit that referenced this pull request Aug 4, 2021
…fective even if DLQ is disabled (#11495)

### Motivation

On the consumer endpoint of WebSocket API, we can specify the delay time before a message which is negatively acknowledged is redelivered using the query parameter `negativeAckRedeliveryDelay`.

However, this parameter is currently ignored when DLQ is disabled. I think this is an implementation mistake. Users should be able to specify `negativeAckRedeliveryDelay` even if DLQ is disabled.
https://github.com/apache/pulsar/blob/ee202d06548e3c73d70ad52374658ee3507ca809/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ConsumerHandler.java#L389-L403

Related PR: #8249

### Modifications

Fixed `ConsumerHandler` of WebSocket to use the `negativeAckRedeliveryDelay` value specified by the client even if DLQ is disabled. In addition, fixed an inappropriate test code (`ProxyPublishConsumeTest#nackMessageTest()`). 

(cherry picked from commit 1f76d0d)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 4, 2021
LeBW pushed a commit to LeBW/pulsar that referenced this pull request Aug 9, 2021
…fective even if DLQ is disabled (apache#11495)

### Motivation

On the consumer endpoint of WebSocket API, we can specify the delay time before a message which is negatively acknowledged is redelivered using the query parameter `negativeAckRedeliveryDelay`.

However, this parameter is currently ignored when DLQ is disabled. I think this is an implementation mistake. Users should be able to specify `negativeAckRedeliveryDelay` even if DLQ is disabled.
https://github.com/apache/pulsar/blob/ee202d06548e3c73d70ad52374658ee3507ca809/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ConsumerHandler.java#L389-L403

Related PR: apache#8249

### Modifications

Fixed `ConsumerHandler` of WebSocket to use the `negativeAckRedeliveryDelay` value specified by the client even if DLQ is disabled. In addition, fixed an inappropriate test code (`ProxyPublishConsumeTest#nackMessageTest()`).
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…fective even if DLQ is disabled (apache#11495)

### Motivation

On the consumer endpoint of WebSocket API, we can specify the delay time before a message which is negatively acknowledged is redelivered using the query parameter `negativeAckRedeliveryDelay`.

However, this parameter is currently ignored when DLQ is disabled. I think this is an implementation mistake. Users should be able to specify `negativeAckRedeliveryDelay` even if DLQ is disabled.
https://github.com/apache/pulsar/blob/ee202d06548e3c73d70ad52374658ee3507ca809/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ConsumerHandler.java#L389-L403

Related PR: apache#8249

### Modifications

Fixed `ConsumerHandler` of WebSocket to use the `negativeAckRedeliveryDelay` value specified by the client even if DLQ is disabled. In addition, fixed an inappropriate test code (`ProxyPublishConsumeTest#nackMessageTest()`).
momo-jun added a commit to momo-jun/pulsar that referenced this pull request Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/websocket cherry-picked/branch-2.8 Archived: 2.8 is end of life doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.8.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants