jt2594838 commented on a change in pull request #1526: URL: https://github.com/apache/incubator-iotdb/pull/1526#discussion_r459193716
########## File path: cluster/src/main/java/org/apache/iotdb/cluster/client/rpcutils/TimeoutChangeableTFastFramedTransport.java ########## @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iotdb.cluster.client.rpcutils; + +import org.apache.thrift.transport.TFastFramedTransport; +import org.apache.thrift.transport.TSocket; +import org.apache.thrift.transport.TTransport; + +public class TimeoutChangeableTFastFramedTransport extends TFastFramedTransport { + + TTransport underlying; + + public TimeoutChangeableTFastFramedTransport(TTransport underlying) { + super(underlying); + this.underlying = underlying; + } + + public void setTimeout(int timeout){ + ((TSocket)underlying).setTimeout(timeout); + } Review comment: It would be better if you enforce the parameter in the constructor to be TSocket, so there will be less chance to trigger a ClassCastException when the code is modified by another person. ########## File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncDataClient.java ########## @@ -39,13 +39,20 @@ public SyncDataClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool) throws TTransportException { // the difference of the two clients lies in the port - super(protocolFactory.getProtocol(new TFastFramedTransport(new TSocket(node.getIp(), - node.getDataPort(), RaftServer.getConnectionTimeoutInMS())))); + super(protocolFactory.getProtocol(new TimeoutChangeableTFastFramedTransport( + new TSocket(node.getIp(), node.getDataPort(), RaftServer.getConnectionTimeoutInMS())))); this.node = node; this.pool = pool; getInputProtocol().getTransport().open(); } + public void setTimeout(int timeout) { + ((TimeoutChangeableTFastFramedTransport) (getInputProtocol().getTransport())) + .setTimeout(timeout); + ((TimeoutChangeableTFastFramedTransport) (getOutputProtocol().getTransport())) + .setTimeout(timeout); Review comment: The same transport is used in both input and output, so it is unnecessary to set twice. ########## File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties ########## @@ -58,7 +58,13 @@ max_concurrent_client_num=10000 default_replica_num=2 # connection time out (ms) among raft nodes -connection_time_out_ms=20000 +connection_timeout_ms=20000 + +# write operation timeout threshold (ms) +write_operation_timeout_ms=30000 + +# read operation timeout threshold (ms) +read_operation_timeout_ms=30000 Review comment: Add comments explaining that the configs are only for internal communications, not for the whole operation. ---------------------------------------------------------------- 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
