[ https://issues.apache.org/jira/browse/BEAM-8557?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
sunjincheng updated BEAM-8557: ------------------------------ Description: 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: ``` @Test public void testUnknownResponseIgnored() throws Exception { String id = "actualInstruction"; String unknownId = "unknownInstruction"; CompletionStage<BeamFnApi.InstructionResponse> responseFuture = client.handle(BeamFnApi.InstructionRequest.newBuilder().setInstructionId(id).build()); client .asResponseObserver() .onNext(BeamFnApi.InstructionResponse.newBuilder().setInstructionId(unknownId).build()); assertThat(MoreFutures.isDone(responseFuture), is(false)); assertThat(MoreFutures.isCancelled(responseFuture), is(false)); } ``` I am do not know why we need test this case? @kennknowles What do you think? was: 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. What do you think? > 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: 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: > ``` > @Test > public void testUnknownResponseIgnored() throws Exception { > String id = "actualInstruction"; > String unknownId = "unknownInstruction"; > CompletionStage<BeamFnApi.InstructionResponse> responseFuture = > > client.handle(BeamFnApi.InstructionRequest.newBuilder().setInstructionId(id).build()); > client > .asResponseObserver() > > .onNext(BeamFnApi.InstructionResponse.newBuilder().setInstructionId(unknownId).build()); > assertThat(MoreFutures.isDone(responseFuture), is(false)); > assertThat(MoreFutures.isCancelled(responseFuture), is(false)); > } > ``` > I am do not know why we need test this case? @kennknowles > What do you think? > -- This message was sent by Atlassian Jira (v8.3.4#803005)