Unreachable dead code in com.alibaba.rocketmq.client.impl.MQClientAPIImpl#processSendResponse

[laosijikaichele]

BUG REPORT Unreachable dead code in com.alibaba.rocketmq.client.impl.MQClientAPIImpl#processSendResponse

The dead code block is :

                case ResponseCode.FLUSH_DISK_TIMEOUT:
                    sendStatus = SendStatus.FLUSH_DISK_TIMEOUT;
                    break;
                case ResponseCode.FLUSH_SLAVE_TIMEOUT:
                    sendStatus = SendStatus.FLUSH_SLAVE_TIMEOUT;
                    break;
                case ResponseCode.SLAVE_NOT_AVAILABLE:
                    sendStatus = SendStatus.SLAVE_NOT_AVAILABLE;
                    break;

[xiangwangcheng]

Indeed, this code block is unreachable. Could you please open a PR and remove it? @dongeforever @zongtanghu

[Switch-vov]

@xiangwangcheng @laosijikaichele

After test it, it is reachable code, but it is very ambiguous. So, I think it should be refactoring to below code.

before update:

    private SendResult processSendResponse(
        final String brokerName,
        final Message msg,
        final RemotingCommand response
    ) throws MQBrokerException, RemotingCommandException {
        switch (response.getCode()) {
            case ResponseCode.FLUSH_DISK_TIMEOUT:
            case ResponseCode.FLUSH_SLAVE_TIMEOUT:
            case ResponseCode.SLAVE_NOT_AVAILABLE: {
            }
            case ResponseCode.SUCCESS: {
                SendStatus sendStatus = SendStatus.SEND_OK;
                switch (response.getCode()) {
                    case ResponseCode.FLUSH_DISK_TIMEOUT:
                        sendStatus = SendStatus.FLUSH_DISK_TIMEOUT;
                        break;
                    case ResponseCode.FLUSH_SLAVE_TIMEOUT:
                        sendStatus = SendStatus.FLUSH_SLAVE_TIMEOUT;
                        break;
                    case ResponseCode.SLAVE_NOT_AVAILABLE:
                        sendStatus = SendStatus.SLAVE_NOT_AVAILABLE;
                        break;
                    case ResponseCode.SUCCESS:
                        sendStatus = SendStatus.SEND_OK;
                        break;
                    default:
                        assert false;
                        break;
                }

                // some code
            }
            default:
                break;
        }

        throw new MQBrokerException(response.getCode(), response.getRemark());
    }

updated code:

    private SendResult processSendResponse(
        final String brokerName,
        final Message msg,
        final RemotingCommand response
    ) throws MQBrokerException, RemotingCommandException {
        SendStatus sendStatus = null;
        switch (response.getCode()) {
            case ResponseCode.FLUSH_DISK_TIMEOUT: {
                sendStatus = SendStatus.FLUSH_DISK_TIMEOUT;
                break;
            }
            case ResponseCode.FLUSH_SLAVE_TIMEOUT: {
                sendStatus = SendStatus.FLUSH_SLAVE_TIMEOUT;
                break;
            }
            case ResponseCode.SLAVE_NOT_AVAILABLE: {
                sendStatus = SendStatus.SLAVE_NOT_AVAILABLE;
                break;
            }
            case ResponseCode.SUCCESS: {
                sendStatus = SendStatus.SEND_OK;
                break;
            }
            default:
                break;
        }

        if (sendStatus != null) {
            // some code
        }

        throw new MQBrokerException(response.getCode(), response.getRemark());
    }

unit test code org.apache.rocketmq.client.impl.MQClientAPIImplTest#testSendMessageSync_WithProcessSendResponseHandleException:

    @Test
    public void testSendMessageSync_WithProcessSendResponseHandleException() throws InterruptedException, RemotingException, MQBrokerException {
        int[] responseCodes = {ResponseCode.FLUSH_DISK_TIMEOUT, ResponseCode.FLUSH_SLAVE_TIMEOUT,
                ResponseCode.SLAVE_NOT_AVAILABLE};
        SendStatus[] sendStatuses = {SendStatus.FLUSH_DISK_TIMEOUT, SendStatus.FLUSH_SLAVE_TIMEOUT, SendStatus.SLAVE_NOT_AVAILABLE};
        for (int i = 0; i < responseCodes.length; i++) {
            final int responseCode = responseCodes[i];
            doAnswer(new Answer() {
                @Override
                public Object answer(InvocationOnMock mock) throws Throwable {
                    RemotingCommand request = mock.getArgument(1);
                    RemotingCommand response = createSuccessResponse(request);
                    response.setCode(responseCode);
                    return response;
                }
            }).when(remotingClient).invokeSync(anyString(), any(RemotingCommand.class), anyLong());

            SendMessageRequestHeader requestHeader = createSendMessageRequestHeader();
            SendResult sendResult = mqClientAPI.sendMessage(brokerAddr, brokerName, msg, requestHeader,
                    3 * 1000, CommunicationMode.SYNC, new SendMessageContext(), defaultMQProducerImpl);
            assertThat(sendResult.getSendStatus()).isEqualTo(sendStatuses[i]);
        }
    }

[Switch-vov]

@xiangwangcheng I apply the PR. please review it. @laosijikaichele I think it should be refactor like the code in the above comment. If remove this code, the code process would be changed.

[github-actions[bot]]

This issue is stale because it has been open for 365 days with no activity. It will be closed in 3 days if no further activity occurs.

[github-actions[bot]]

This issue was closed because it has been inactive for 3 days since being marked as stale.