Matthew Jacobs 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 shar Done 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? I don't think there's a good way to set globals across java and the C++ code. 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? Hm, I see this in a number of files I haven't touched, and my eclipse organizes like this - I'm guessing other ppl do too. PS2, Line 148: Preconditions.checkState(parsedReplicas > 0); > Maybe add an if-check and throw a RuntimeException to make the error better This should already be checked and thrown in a nice way by analysis. I think this is fair game for a Precondition, though I can add a string to this here. 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' Done Line 63: return b.build(); > can this call already time out? I don't think this makes any RPCs 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? Done Line 70: @CustomClusterTestSuite.with_args( > single line? Done Line 76: > nit: remove newlines at end of file Done -- 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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
