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/11/26 14:14:08 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request #8724: [WebSocket] Fix the initial sequence id error
BewareMyPower opened a new pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724
#### Motivation
The WebSocket `ProducerHandler` parse a wrong string so the client's query param `initialSequenceId` will always fail.
### Modifications
Parse the real `initialSequenceId` value.
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531361573
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
Since the exception is thrown by `getProducerBuilder`, I think I can add a test to check if any `NumberFormatException` or `IllegalArgumentException` was thrown by the `ProducerHandler`'s constructor and treat other exceptions as right behavior.
----------------------------------------------------------------
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 merged pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531359612
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
But `ProducerBuilder` just provides getters but not setters. So we need to call `create` and it will fail if `PulsarClient`'s service url is not correct.
----------------------------------------------------------------
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] zymap commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531361848
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
You can use`mock` to get every behavior you want.
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#issuecomment-734580566
/pulsarbot run-failure-checks
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531360499
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
In addition, `getProducerBuilder` is private. Even if change it to `public`, it must be called after `ProducerHandler` is constructed. However, the constructor calls `getProducerBuilder` and `ProducerBuilder#create`. This behavior cannot be changed even with a mocked `WebSocketService` and `HttpServletRequest`.
----------------------------------------------------------------
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] zymap commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531359174
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
I mean something like this:
```
@Test
public void testGetTheRightBuilder() {
HttpServletRequest request = mock(HttpServletRequest.class);
when(request.getParameterMap()).thenReturn(mockedmap)
ProducerHandler producerHandler = new ProducerHandler(null, request, null);
ProducerBuilderImpl<byte[]> builder = (ProducerBuilderImpl<byte[]>) producerHandler.getProducerBuilder(null);
builder.getConf().getProducerName();
}
```
----------------------------------------------------------------
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] zymap commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531362385
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
If you need to use the `getProducerBuilder` method, I think it's ok to make it to package private not public.
----------------------------------------------------------------
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] zymap commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531356104
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
We don't need to validate the params. We can simply mock the queryParams which is a hashmap and put the arguments into the map. Then when we can get the values in the `getProducerBuilder`. We can make sure the ProducerBuilder contains the configurations which we pass to the hashmap.
Currently, the `getProducerBuilder` can not get the right builder with the `queryParams`, so we can make sure it can get the right builder with the given params.
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531361573
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
Since the exception is thrown by `getProducerBuilder`, I think I can add a test to check if any `NumberFormatException` or `IllegalArgumentException`.
----------------------------------------------------------------
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] zymap commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531260745
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
Add a test for `getProducerBuilder` methods? We can verify the producer applied all we gave configurations.
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#issuecomment-734545217
/pulsarbot run-failure-checks
----------------------------------------------------------------
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] zymap commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531361236
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
I am not sure can we cast `ProduceBuilder` to `ProducerBuilderImpl` to get some properties?
```
ProducerBuilderImpl<byte[]> builder = (ProducerBuilderImpl<byte[]>) producerHandler.getProducerBuilder(null);
builder.getConf().getProducerName();
```
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531349653
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
The `getProducerBuilder` cannot be tested simply. The parameter validation happens in `ProducerBuilder#create`. However the `PulsarClient`'s service url cannot be mocked because it retrieves the url related params from ZK, see `WebSocketService#getPulsarClient` for details.
Therefore, it needs an integration test which may be beyond this PR's scope of work. Also I found there's no WebSocket related integration tests. If you think it's proper to add an WebSocket integration test in this PR, I'll take some time to do 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] BewareMyPower commented on pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#issuecomment-734719505
/pulsarbot run-failure-checks
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#discussion_r531372496
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ProducerHandler.java
##########
@@ -298,7 +298,7 @@ private void updateSentMsgStats(long msgSize, long latencyUsec) {
}
if (queryParams.containsKey("initialSequenceId")) {
- builder.initialSequenceId(Long.parseLong("initialSequenceId"));
+ builder.initialSequenceId(Long.parseLong(queryParams.get("initialSequenceId")));
Review comment:
Ok, I'll add a test soon.
----------------------------------------------------------------
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 #8724: [WebSocket] Fix the initial sequence id error
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8724:
URL: https://github.com/apache/pulsar/pull/8724#issuecomment-735527094
/pulsarbot cherry-pick to branch-2.7
----------------------------------------------------------------
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