[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=34=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-34
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 15/Nov/19 17:41
Start Date: 15/Nov/19 17:41
Worklog Time Spent: 10m
Work Description: lukecwik commented on pull request #9991:
[BEAM-8557]Add log for the dropped unknown response
URL: https://github.com/apache/beam/pull/9991
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 34)
Time Spent: 2h 20m (was: 2h 10m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=32=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-32
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 15/Nov/19 17:40
Start Date: 15/Nov/19 17:40
Worklog Time Spent: 10m
Work Description: lukecwik commented on pull request #9991:
[BEAM-8557]Add log for the dropped unknown response
URL: https://github.com/apache/beam/pull/9991#discussion_r346935385
##
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java
##
@@ -148,16 +148,14 @@ public void onNext(BeamFnApi.InstructionResponse
response) {
LOG.debug("Received InstructionResponse {}", response);
CompletableFuture responseFuture =
outstandingRequests.remove(response.getInstructionId());
- if (responseFuture != null) {
-if (response.getError().isEmpty()) {
- responseFuture.complete(response);
-} else {
- responseFuture.completeExceptionally(
- new RuntimeException(
- String.format(
- "Error received from SDK harness for instruction %s: %s",
- response.getInstructionId(), response.getError(;
-}
+ if (response.getError().isEmpty()) {
Review comment:
It is an error if we receive a response for something we weren't expecting.
We should close the control/data/state channel. The runner is then
responsible for restarting the SDK harness and resuming processing.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 32)
Time Spent: 2h 10m (was: 2h)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=33=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-33
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 15/Nov/19 17:40
Start Date: 15/Nov/19 17:40
Worklog Time Spent: 10m
Work Description: lukecwik commented on pull request #9991:
[BEAM-8557]Add log for the dropped unknown response
URL: https://github.com/apache/beam/pull/9991#discussion_r346935922
##
File path:
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/state/BeamFnStateGrpcClientCache.java
##
@@ -138,12 +138,14 @@ private synchronized void
closeAndCleanUp(RuntimeException cause) {
public void onNext(StateResponse value) {
LOG.debug("Received StateResponse {}", value);
CompletableFuture responseFuture =
outstandingRequests.remove(value.getId());
-if (responseFuture != null) {
- if (value.getError().isEmpty()) {
-responseFuture.complete(value);
- } else {
-responseFuture.completeExceptionally(new
IllegalStateException(value.getError()));
- }
+if (responseFuture == null) {
Review comment:
this should be a terminal error on the SDK side where we should close the
control/data/state channel and have the runner restart us
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 33)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=341542=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-341542
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 11/Nov/19 23:45
Start Date: 11/Nov/19 23:45
Worklog Time Spent: 10m
Work Description: kennknowles commented on pull request #9991:
[BEAM-8557]Add log for the dropped unknown response
URL: https://github.com/apache/beam/pull/9991#discussion_r344959866
##
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java
##
@@ -158,6 +158,8 @@ public void onNext(BeamFnApi.InstructionResponse response)
{
"Error received from SDK harness for instruction %s: %s",
response.getInstructionId(), response.getError(;
}
+ } else {
+LOG.warn("Dropped unknown InstructionResponse {}", response);
Review comment:
Tiny style suggestion: move the error case to the top. Reduce one level of
curly braces.
```java
if (responseFuture == null) {
LOG.warn(...)
return;
}
```
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 341542)
Time Spent: 2h (was: 1h 50m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 2h
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=341296=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-341296
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 11/Nov/19 13:23
Start Date: 11/Nov/19 13:23
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on issue #9991: [BEAM-8557]Add
log for the dropped unknown response
URL: https://github.com/apache/beam/pull/9991#issuecomment-552443631
R: @kennknowles I have updated the PR, is that make sense to you :) Welcome
any feedback.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 341296)
Time Spent: 1h 50m (was: 1h 40m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339690=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339690
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 07/Nov/19 02:03
Start Date: 07/Nov/19 02:03
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r343423145
##
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java
##
@@ -148,16 +148,14 @@ public void onNext(BeamFnApi.InstructionResponse
response) {
LOG.debug("Received InstructionResponse {}", response);
CompletableFuture responseFuture =
outstandingRequests.remove(response.getInstructionId());
- if (responseFuture != null) {
-if (response.getError().isEmpty()) {
- responseFuture.complete(response);
-} else {
- responseFuture.completeExceptionally(
- new RuntimeException(
- String.format(
- "Error received from SDK harness for instruction %s: %s",
- response.getInstructionId(), response.getError(;
-}
+ if (response.getError().isEmpty()) {
Review comment:
Totally agree that this may mean a bug in the SDK harness if it happens. I'm
just thinking that a log message may be not enough as it's easy to be ignored.
An exception makes sure that problem was discovered in time. But I'm fine if
you think log is enough as users will investigate the log if there are serious
problem for them after all. So, we need only add a WARNING log for this case,
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339690)
Time Spent: 1h 40m (was: 1.5h)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339532=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339532
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 18:29
Start Date: 06/Nov/19 18:29
Worklog Time Spent: 10m
Work Description: kennknowles commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r343253895
##
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java
##
@@ -148,16 +148,14 @@ public void onNext(BeamFnApi.InstructionResponse
response) {
LOG.debug("Received InstructionResponse {}", response);
CompletableFuture responseFuture =
outstandingRequests.remove(response.getInstructionId());
- if (responseFuture != null) {
-if (response.getError().isEmpty()) {
- responseFuture.complete(response);
-} else {
- responseFuture.completeExceptionally(
- new RuntimeException(
- String.format(
- "Error received from SDK harness for instruction %s: %s",
- response.getInstructionId(), response.getError(;
-}
+ if (response.getError().isEmpty()) {
Review comment:
I read some code and thought about this. The message comes from the other
side of a gRPC connection. We cannot assume what code is connected. It could be
any of the SDK harnesses, or could be another SDK harness not part of Beam.
Receiving a response for a request where we are not waiting for an answer is
OK. Maybe it means there is a bug in code on the other side of the connection.
I don't think the runner side should crash and terminate. It may be worth a log
message, but then a problem on the SDK harness could cause flooding on the
runner.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339532)
Time Spent: 1.5h (was: 1h 20m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339265=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339265
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 09:39
Start Date: 06/Nov/19 09:39
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on issue #9991: [BEAM-8557]
Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#issuecomment-550228265
Run Portable_Python PreCommit
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339265)
Time Spent: 1h 20m (was: 1h 10m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339186=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339186
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 06:18
Start Date: 06/Nov/19 06:18
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r342931236
##
File path:
runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/control/FnApiControlClientTest.java
##
@@ -100,7 +99,8 @@ public void testRequestError() throws Exception {
}
@Test
- public void testUnknownResponseIgnored() throws Exception {
+ public void testUnknownResponse() throws Exception {
+thrown.expect(NullPointerException.class);
Review comment:
I agree that meaningful message is better than NPE.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339186)
Time Spent: 1h 10m (was: 1h)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339184=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339184
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 06:17
Start Date: 06/Nov/19 06:17
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r342931236
##
File path:
runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/control/FnApiControlClientTest.java
##
@@ -100,7 +99,8 @@ public void testRequestError() throws Exception {
}
@Test
- public void testUnknownResponseIgnored() throws Exception {
+ public void testUnknownResponse() throws Exception {
+thrown.expect(NullPointerException.class);
Review comment:
I agree that meaningful message is better than NPE.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339184)
Time Spent: 50m (was: 40m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 50m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339183=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339183
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 06:17
Start Date: 06/Nov/19 06:17
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r342931221
##
File path:
runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/control/FnApiControlClientTest.java
##
@@ -100,7 +99,8 @@ public void testRequestError() throws Exception {
}
@Test
- public void testUnknownResponseIgnored() throws Exception {
+ public void testUnknownResponse() throws Exception {
+thrown.expect(NullPointerException.class);
Review comment:
I agree that meaningful message is better than NPE.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339183)
Time Spent: 40m (was: 0.5h)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339185=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339185
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 06:17
Start Date: 06/Nov/19 06:17
Worklog Time Spent: 10m
Work Description: sunjincheng121 commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r342931269
##
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java
##
@@ -148,16 +148,14 @@ public void onNext(BeamFnApi.InstructionResponse
response) {
LOG.debug("Received InstructionResponse {}", response);
CompletableFuture responseFuture =
outstandingRequests.remove(response.getInstructionId());
- if (responseFuture != null) {
-if (response.getError().isEmpty()) {
- responseFuture.complete(response);
-} else {
- responseFuture.completeExceptionally(
- new RuntimeException(
- String.format(
- "Error received from SDK harness for instruction %s: %s",
- response.getInstructionId(), response.getError(;
-}
+ if (response.getError().isEmpty()) {
Review comment:
Thanks for the quick reply!
Is there some logic for the resent the message? I have not find it, so I
think in this case there is no duplicate message.
Please correct me if there anything I misunderstand :)
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339185)
Time Spent: 1h (was: 50m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339175=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339175
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 06:03
Start Date: 06/Nov/19 06:03
Worklog Time Spent: 10m
Work Description: kennknowles commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r342927908
##
File path:
runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/control/FnApiControlClientTest.java
##
@@ -100,7 +99,8 @@ public void testRequestError() throws Exception {
}
@Test
- public void testUnknownResponseIgnored() throws Exception {
+ public void testUnknownResponse() throws Exception {
+thrown.expect(NullPointerException.class);
Review comment:
My opinion: an NPE always indicates a programming error. Null should not
exist anymore in high-level programming. Or in most medium-level or low-level
programming. The code should prevent an NPE and give a good error message.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339175)
Time Spent: 20m (was: 10m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[
https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=339176=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-339176
]
ASF GitHub Bot logged work on BEAM-8557:
Author: ASF GitHub Bot
Created on: 06/Nov/19 06:03
Start Date: 06/Nov/19 06:03
Worklog Time Spent: 10m
Work Description: kennknowles commented on pull request #9991:
[BEAM-8557] Cleanup the useless null check for ResponseStreamObserver…
URL: https://github.com/apache/beam/pull/9991#discussion_r342928301
##
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java
##
@@ -148,16 +148,14 @@ public void onNext(BeamFnApi.InstructionResponse
response) {
LOG.debug("Received InstructionResponse {}", response);
CompletableFuture responseFuture =
outstandingRequests.remove(response.getInstructionId());
- if (responseFuture != null) {
-if (response.getError().isEmpty()) {
- responseFuture.complete(response);
-} else {
- responseFuture.completeExceptionally(
- new RuntimeException(
- String.format(
- "Error received from SDK harness for instruction %s: %s",
- response.getInstructionId(), response.getError(;
-}
+ if (response.getError().isEmpty()) {
Review comment:
I do not like this change, because of my comment below about NPE.
But also, I do not know the code well enough. Is there a distributed systems
question? Can a duplicate message be received that should be recieved?
If the answer is "yes" then the previous code is correct. If the answer is
"no" then the previous code should crash or at least log an error.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 339176)
Time Spent: 0.5h (was: 20m)
> Clean up useless null check.
>
>
> Key: BEAM-8557
> URL: https://issues.apache.org/jira/browse/BEAM-8557
> Project: Beam
> Issue Type: Sub-task
> Components: runner-core, sdk-java-harness
>Reporter: sunjincheng
>Assignee: sunjincheng
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> I think we do not need null check here:
> [https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClient.java#L151]
> Because before the the `onNext` call, the `Future` already put into the queue
> in `handle` method.
>
> I found the test as follows:
> {code:java}
> @Test
> public void testUnknownResponseIgnored() throws Exception{code}
> I do not know why we need test this case? I think it would be better if we
> throw the Exception for an UnknownResponse, otherwise, this may hidden a
> potential bug.
> Please correct me if there anything I misunderstand @kennknowles
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-8557) Clean up useless null check.
[ https://issues.apache.org/jira/browse/BEAM-8557?focusedWorklogId=338637=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-338637 ] ASF GitHub Bot logged work on BEAM-8557: Author: ASF GitHub Bot Created on: 05/Nov/19 10:01 Start Date: 05/Nov/19 10:01 Worklog Time Spent: 10m Work Description: sunjincheng121 commented on pull request #9991: [BEAM-8557] Cleanup the useless null check for ResponseStreamObserver… URL: https://github.com/apache/beam/pull/9991 In this PR will remove the null check for `ResponseStreamObserver` and `InboundObserver` Because before the the `onNext` call, the `Future` already put into the queue in `handle` method. Post-Commit Tests Status (on master branch) Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark --- | --- | --- | --- | --- | --- | --- | --- Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/) Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [