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

Nack support in WS #8249

Merged
merged 12 commits into from
May 21, 2021
Merged

Nack support in WS #8249

merged 12 commits into from
May 21, 2021

Conversation

yrsh
Copy link
Contributor

@yrsh yrsh commented Oct 13, 2020

Contribution Checklist

Fixes #8247

Motivation

Nack support in WS API.

Modifications

handleNack method in ConsumerHandler and additional parameter for consumer negativeAckRedeliveryDelay.

Current WS API test is ok.

Copy link
Contributor

@Huanli-Meng Huanli-Meng left a comment

Choose a reason for hiding this comment

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

if the doc in version 2.2.0 is updated. I think docs in all releases since 2.2.0 should be updated too. Could you please help update docs for the rest releases. Thanks.

…bsocket.md

Co-authored-by: HuanliMeng <48120384+Huanli-Meng@users.noreply.github.com>
@yrsh
Copy link
Contributor Author

yrsh commented Oct 17, 2020

@Huanli-Meng it looks like my mistake, I didn't notice that I updated the docs for 2.2. Early versions do not have this feature, I think it may confuse users. So only the latest docks need to be updated. I.e. site2/docs/client-libraries-websocket.md as I understand it. What do you think about it?

@Huanli-Meng
Copy link
Contributor

@Huanli-Meng it looks like my mistake, I didn't notice that I updated the docs for 2.2. Early versions do not have this feature, I think it may confuse users. So only the latest docks need to be updated. I.e. site2/docs/client-libraries-websocket.md as I understand it. What do you think about it?

No worries. You are right. if your updates is only for the latest release, you should only update site2/docs/client-libraries-websocket.md.

@yrsh
Copy link
Contributor Author

yrsh commented Oct 18, 2020

@Huanli-Meng ok, now only the latest docs are updated.

Copy link
Contributor

@Huanli-Meng Huanli-Meng left a comment

Choose a reason for hiding this comment

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

LGTM for doc changes. For code updates, let's wait for other engineer's comments, if any.

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@yrsh The change looks great to me! Can you add a test case for it? Examples are in https://github.com/apache/pulsar/tree/master/pulsar-websocket/src/test/java/org/apache/pulsar/websocket

@sijie sijie added area/websocket type/feature The PR added a new feature or issue requested a new feature and removed area/client labels Oct 21, 2020
@yrsh
Copy link
Contributor Author

yrsh commented Oct 21, 2020

@sijie Thanks! I thought about tests, but when I did this feature I did not figure out how to make it. Because I don't see there tests that directly check the work of handlers. For example, there are no ack test in AbstractWebSocketHandlerTest. To check the nack, in theory we should accept the message, reject it, and then make sure that it appeared in DLQ. I.e. this is functional test, not unit. Can you explain me what test case should I do in current solution?

@sijie
Copy link
Member

sijie commented Oct 21, 2020

For example, there are no ack test in AbstractWebSocketHandlerTest. To check the nack, in theory we should accept the message, reject it, and then make sure that it appeared in DLQ. I.e. this is functional test, not unit. Can you explain me what test case should I do in current solution?

Good point! I was pointing to the wrong package. You can find the related tests at this package: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/ProxyPublishConsumeTest.java

It is a functional test.

@codelipenghui
Copy link
Contributor

@yrsh Could you please take a look at the last comment?

@yrsh
Copy link
Contributor Author

yrsh commented Nov 9, 2020

@codelipenghui Unfortunately I don't have time to finish this task now, I plan to finish writing tests in the next few days.

@yrsh
Copy link
Contributor Author

yrsh commented Nov 10, 2020

@sijie I added a test, but there are some weirdness. When I call produceSocket.sendMessage(1); to my test topic, I get 11 messages in it.

java.lang.AssertionError: expected [1] but found [11]

So at the beginning I check that the topic is empty, and then that there are messages in the dlq, because now I don't know how it's work or maybe it's bug(?).

@codelipenghui
Copy link
Contributor

move to 2.8.0 first.

@yrsh
Copy link
Contributor Author

yrsh commented Dec 3, 2020

What is the blocker for merge?

@yrsh
Copy link
Contributor Author

yrsh commented Dec 21, 2020

@Huanli-Meng @codelipenghui
The request has been hanging for more than two weeks, is there something wrong?

@Huanli-Meng
Copy link
Contributor

@codelipenghui , could you help double check this PR? No more comments for doc updates from me. Thanks

@yrsh
Copy link
Contributor Author

yrsh commented Jan 6, 2021

@codelipenghui, could you clarify what the problem is? the PR has been hanging for more than a month

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Looks good to me, please help take a look at the license header.

@@ -0,0 +1,7 @@
package org.apache.pulsar.websocket.proxy;
Copy link
Contributor

Choose a reason for hiding this comment

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

Miss license header here.

@codelipenghui codelipenghui merged commit 2be05fb into apache:master May 21, 2021
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
sijie 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()`).
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)
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
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()`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/websocket type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nack support in WS
5 participants