Lars Volker has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/4849/2/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: PS2, Line 123: kudu::MonoDelta timeout = kudu::MonoDelta::FromMilliseconds( : FLAGS_kudu_client_timeout_ms); : b.default_rpc_timeout(timeout); : b.default_admin_operation_timeout(timeout); : : KUDU_RETURN_IF_ERROR(b.Build(&client_), "Unable to create Kudu client"); This seems to be duplicated in the scan node. Can you factor it into a shared method? http://gerrit.cloudera.org:8080/#/c/4849/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: Line 43: private static int kuduClientTimeoutMs_ = 3 * 60 * 1000; This is also the default in global-flags.cc. Do we need to repeat it here? http://gerrit.cloudera.org:8080/#/c/4849/2/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 43: import com.google.common.base.Preconditions; Nit: wouldn't com come before org, alphabetically? PS2, Line 148: Preconditions.checkState(parsedReplicas > 0); Maybe add an if-check and throw a RuntimeException to make the error better understandable to the user? http://gerrit.cloudera.org:8080/#/c/4849/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: PS2, Line 54: k Nit: upper case 'Kudu' Line 63: return b.build(); can this call already time out? http://gerrit.cloudera.org:8080/#/c/4849/2/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: Line 63: @CustomClusterTestSuite.with_args( single line? Line 70: @CustomClusterTestSuite.with_args( single line? Line 76: nit: remove newlines at end of file -- To view, visit http://gerrit.cloudera.org:8080/4849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-HasComments: Yes
