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 2020/10/13 17:29:55 UTC
[GitHub] [pulsar] yrsh opened a new pull request #8249: Nack support in WS
yrsh opened a new pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249
### 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.
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh edited a comment on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh edited a comment on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-724643261
@sijie I added a test, but there are some weirdness. When I call [produceSocket.sendMessage(1);](https://github.com/apache/pulsar/pull/8249/files#diff-69013dd81a8eefa2e70e8895f28010dc26aebc53853a780fe707603c33fb068fR626) 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(?).
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-737985896
What is the blocker for merge?
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh edited a comment on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh edited a comment on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-710769623
@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. I think only the latest docks need to be updated. I.e. site2/docs/client-libraries-websocket.md as I understand it. What do you thing about it?
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh edited a comment on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh edited a comment on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-710769623
@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?
----------------------------------------------------------------
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
[GitHub] [pulsar] codelipenghui commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-721749319
@yrsh Could you please take a look at the last comment?
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-710769623
@Huanli-Meng it looks like my mistake, I didn't notice that I updated the docks for 2.2. Early versions do not have this feature, I think it may confuse users. I think only the latest docks need to be updated. I.e. site2/docs/client-libraries-websocket.md as I understand it. What do you thing about it?
----------------------------------------------------------------
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
[GitHub] [pulsar] Huanli-Meng commented on a change in pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
Huanli-Meng commented on a change in pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#discussion_r507156135
##########
File path: site2/docs/client-libraries-websocket.md
##########
@@ -145,6 +145,8 @@ Key | Type | Required? | Explanation
`maxRedeliverCount` | int | no | Define a [maxRedeliverCount](http://pulsar.apache.org/api/client/org/apache/pulsar/client/api/ConsumerBuilder.html#deadLetterPolicy-org.apache.pulsar.client.api.DeadLetterPolicy-) for the consumer (default: 0). Activates [Dead Letter Topic](https://github.com/apache/pulsar/wiki/PIP-22%3A-Pulsar-Dead-Letter-Topic) feature.
`deadLetterTopic` | string | no | Define a [deadLetterTopic](http://pulsar.apache.org/api/client/org/apache/pulsar/client/api/ConsumerBuilder.html#deadLetterPolicy-org.apache.pulsar.client.api.DeadLetterPolicy-) for the consumer (default: {topic}-{subscription}-DLQ). Activates [Dead Letter Topic](https://github.com/apache/pulsar/wiki/PIP-22%3A-Pulsar-Dead-Letter-Topic) feature.
`pullMode` | boolean | no | Enable pull mode (default: false). See "Flow Control" below.
+`negativeAckRedeliveryDelay` | int | no | When a negative acked message will be redelivered to DLQ.
Review comment:
Change this sentence "When a negative acked message will be redelivered to DLQ." to "A negatively-acknowledged message will be redelivered to the DLQ." or "When a message is negatively acknowledged, it will be redelivered to the DLQ." Does this make sense? 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
[GitHub] [pulsar] codelipenghui commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-728659477
move to 2.8.0 first.
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-711134888
@Huanli-Meng ok, now only the latest docs are updated.
----------------------------------------------------------------
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
[GitHub] [pulsar] Huanli-Meng commented on a change in pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
Huanli-Meng commented on a change in pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#discussion_r506477078
##########
File path: site2/website/versioned_docs/version-2.2.0/client-libraries-websocket.md
##########
@@ -180,6 +181,17 @@ Key | Type | Required? | Explanation
:---|:-----|:----------|:-----------
`messageId`| string | yes | Message ID of the processed message
+#### Negative acknowledging the message
Review comment:
```suggestion
#### Negatively acknowledging messages
```
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh edited a comment on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh edited a comment on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-724643261
@sijie I added a test, but there are some weirdness. When I call [produceSocket.sendMessage(1);](https://github.com/apache/pulsar/pull/8249/files#diff-69013dd81a8eefa2e70e8895f28010dc26aebc53853a780fe707603c33fb068fR621) to my test topic, I get 11 messages in it.
```
java.lang.AssertionError: expected [1] but found [11]
```
So now it is just checked that there are more than zero, because now I don't know how it's work.
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-748818847
@Huanli-Meng @codelipenghui
The request has been hanging for more than two weeks, is there something wrong?
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-755752888
@codelipenghui, could you clarify what the problem is? the PR has been hanging for more than a month
----------------------------------------------------------------
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
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#discussion_r524858103
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/ProxyPublishConsumeTest.java
##########
@@ -564,6 +566,76 @@ public void socketPullModeTest() throws Exception {
}
}
+ @Test(timeOut = 10000)
+ public void nackMessageTest() throws Exception {
+ final String subscription = "my-sub";
+ final String dlqTopic = "my-property/my-ns/my-topic10";
+ final String consumerTopic = "my-property/my-ns/my-topic9";
+
+ final String dlqUri = "ws://localhost:" + proxyServer.getListenPortHTTP().get() +
+ "/ws/v2/consumer/persistent/" +
+ dlqTopic + "/" + subscription +
+ "?subscriptionType=Shared";
+
+ final String consumerUri = "ws://localhost:" + proxyServer.getListenPortHTTP().get() +
+ "/ws/v2/consumer/persistent/" +
+ consumerTopic + "/" + subscription +
+ "?deadLetterTopic=" + dlqTopic +
+ "&maxRedeliverCount=0&subscriptionType=Shared&ackTimeoutMillis=1000&negativeAckRedeliveryDelay=1000";
+
+ final String producerUri = "ws://localhost:" + proxyServer.getListenPortHTTP().get() +
+ "/ws/v2/producer/persistent/" + consumerTopic;
+
+ WebSocketClient consumeClient1 = new WebSocketClient();
+ SimpleConsumerSocket consumeSocket1 = new SimpleConsumerSocket();
+ WebSocketClient consumeClient2 = new WebSocketClient();
+ SimpleConsumerSocket consumeSocket2 = new SimpleConsumerSocket();
+ WebSocketClient produceClient = new WebSocketClient();
+ SimpleProducerSocket produceSocket = new SimpleProducerSocket();
+
+ consumeSocket1.setMessageHandler((id, data) -> {
+ JsonObject nack = new JsonObject();
+ nack.add("messageId", new JsonPrimitive(id));
+ nack.add("type", new JsonPrimitive("negativeAcknowledge"));
+ return nack.toString();
+ });
+
+ try {
+ consumeClient1.start();
+ consumeClient2.start();
+ ClientUpgradeRequest consumeRequest1 = new ClientUpgradeRequest();
+ ClientUpgradeRequest consumeRequest2 = new ClientUpgradeRequest();
+ Future<Session> consumerFuture1 = consumeClient1.connect(consumeSocket1, URI.create(consumerUri), consumeRequest1);
+ Future<Session> consumerFuture2 = consumeClient2.connect(consumeSocket2, URI.create(dlqUri), consumeRequest2);
+
+ assertTrue(consumerFuture1.get().isOpen());
+ assertTrue(consumerFuture2.get().isOpen());
+
+ ClientUpgradeRequest produceRequest = new ClientUpgradeRequest();
+ produceClient.start();
+ Future<Session> producerFuture = produceClient.connect(produceSocket, URI.create(producerUri), produceRequest);
+ assertTrue(producerFuture.get().isOpen());
+
+ assertEquals(consumeSocket1.getReceivedMessagesCount(), 0);
+ assertEquals(consumeSocket2.getReceivedMessagesCount(), 0);
+
+ produceSocket.sendMessage(1);
+
+ Thread.sleep(500);
+
+ //assertEquals(consumeSocket1.getReceivedMessagesCount(), 1);
Review comment:
I think you can refine the test as https://github.com/apache/pulsar/pull/8557 does. Sleep also introduces too much flaky 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
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#discussion_r560965409
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/SimpleConsumerMessageHandler.java
##########
@@ -0,0 +1,7 @@
+package org.apache.pulsar.websocket.proxy;
Review comment:
Miss license header here.
----------------------------------------------------------------
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
[GitHub] [pulsar] Huanli-Meng commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
Huanli-Meng commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-748872797
@codelipenghui , could you help double check this PR? No more comments for doc updates from me. 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
[GitHub] [pulsar] codelipenghui merged pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249
--
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-724643261
@sijie I added a test, but there are some weirdness. When I call [produceSocket.sendMessage(1);](https://github.com/apache/pulsar/pull/8249/files#diff-69013dd81a8eefa2e70e8895f28010dc26aebc53853a780fe707603c33fb068fR621) to my test topic, I get 11 messages in it.
```
java.lang.AssertionError: expected [1] but found [11]
```
So now it is just checked that there are more than zero
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on a change in pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on a change in pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#discussion_r507175417
##########
File path: site2/docs/client-libraries-websocket.md
##########
@@ -145,6 +145,8 @@ Key | Type | Required? | Explanation
`maxRedeliverCount` | int | no | Define a [maxRedeliverCount](http://pulsar.apache.org/api/client/org/apache/pulsar/client/api/ConsumerBuilder.html#deadLetterPolicy-org.apache.pulsar.client.api.DeadLetterPolicy-) for the consumer (default: 0). Activates [Dead Letter Topic](https://github.com/apache/pulsar/wiki/PIP-22%3A-Pulsar-Dead-Letter-Topic) feature.
`deadLetterTopic` | string | no | Define a [deadLetterTopic](http://pulsar.apache.org/api/client/org/apache/pulsar/client/api/ConsumerBuilder.html#deadLetterPolicy-org.apache.pulsar.client.api.DeadLetterPolicy-) for the consumer (default: {topic}-{subscription}-DLQ). Activates [Dead Letter Topic](https://github.com/apache/pulsar/wiki/PIP-22%3A-Pulsar-Dead-Letter-Topic) feature.
`pullMode` | boolean | no | Enable pull mode (default: false). See "Flow Control" below.
+`negativeAckRedeliveryDelay` | int | no | When a negative acked message will be redelivered to DLQ.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-713665152
> 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.
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-723849886
@codelipenghui Unfortunately I don't have time to finish this task now, I plan to finish writing tests in the next few days.
----------------------------------------------------------------
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
[GitHub] [pulsar] Huanli-Meng commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
Huanli-Meng commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-710928120
> @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.
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh commented on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh commented on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-713570257
@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?
----------------------------------------------------------------
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
[GitHub] [pulsar] yrsh edited a comment on pull request #8249: Nack support in WS
Posted by GitBox <gi...@apache.org>.
yrsh edited a comment on pull request #8249:
URL: https://github.com/apache/pulsar/pull/8249#issuecomment-710769623
@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. I think 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?
----------------------------------------------------------------
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