[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. IMPALA-3771: Expose kudu client timeout and set default The Kudu client timeout was too low for Impala usage. This sets the default timeout to 3 minutes and exposes it as a gflag. New timeout tests were added. Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 Reviewed-on: http://gerrit.cloudera.org:8080/4849 Reviewed-by: Matthew JacobsTested-by: Internal Jenkins --- M be/src/catalog/catalog.cc M be/src/common/global-flags.cc M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/exec/kudu-scanner.cc M be/src/exec/kudu-table-sink.cc M be/src/exec/kudu-table-sink.h M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/service/frontend.cc M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test M tests/common/impala_test_suite.py M tests/custom_cluster/test_kudu.py 21 files changed, 209 insertions(+), 101 deletions(-) Approvals: Matthew Jacobs: Looks good to me, approved Internal Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Internal Jenkins has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 6: Verified+1 -- 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 6: Code-Review+2 -- 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Dan Hecht has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/4849/5/testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test File testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test: Line 7: Unable to initialize the Kudu scan node > Hmm yes, I suppose this (and in the other file) could be flaky. I'm not sur #2 is fine with me, as long as there are comments. -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/4849/5/be/src/exec/kudu-table-sink.h File be/src/exec/kudu-table-sink.h: Line 103: /// This uses 'kudu::client::sp::shared_ptr' as that is the type expected by Kudu. > this comment can probably be removed now that kudu has it's own namespace w Done http://gerrit.cloudera.org:8080/#/c/4849/5/testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test File testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test: Line 7: Unable to initialize the Kudu scan node > will these always give the error? couldn't it possibly work with 1ms timeou Hmm yes, I suppose this (and in the other file) could be flaky. I'm not sure there'll be any way to _ensure_ that any of these operations take longer than 1ms. I can think of these options: 1) take these tests out. not the end of the world but nice to have this coverage. 2) leave them in (and add comments), maybe it'll never be an issue in practice. I can remove/xfail them later if necessary, or see if the Kudu folks have ideas about how to make it time out. I guess i'd prefer to stick with #2 for now since it seems likely that all of these operations will actually timeout (all metadata ops involving the master, constructing a response with a bunch of strings to serialize, etc), and the coverage is nice. What do you think? -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Dan Hecht has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 5: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/4849/5/be/src/exec/kudu-table-sink.h File be/src/exec/kudu-table-sink.h: Line 103: /// This uses 'kudu::client::sp::shared_ptr' as that is the type expected by Kudu. this comment can probably be removed now that kudu has it's own namespace which makes it clear what's going on. http://gerrit.cloudera.org:8080/#/c/4849/5/testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test File testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test: Line 7: Unable to initialize the Kudu scan node will these always give the error? couldn't it possibly work with 1ms timeout? -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Alex Behm has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 5: +2 for FE, +1 for BE -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Alex Behm has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 5: Code-Review+1 Thanks. The timeout is clearer now. -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Hello Lars Volker, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4849 to look at the new patch set (#5). Change subject: IMPALA-3771: Expose kudu client timeout and set default .. IMPALA-3771: Expose kudu client timeout and set default The Kudu client timeout was too low for Impala usage. This sets the default timeout to 3 minutes and exposes it as a gflag. New timeout tests were added. Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 --- M be/src/catalog/catalog.cc M be/src/common/global-flags.cc M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/exec/kudu-scanner.cc M be/src/exec/kudu-table-sink.cc M be/src/exec/kudu-table-sink.h M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/service/frontend.cc M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test M tests/common/impala_test_suite.py M tests/custom_cluster/test_kudu.py 21 files changed, 200 insertions(+), 95 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/4849/5 -- To view, visit http://gerrit.cloudera.org:8080/4849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: Line 44: DECLARE_int32(kudu_client_timeout_ms); > kudu_client_rpc_timeout_ms? or what exactly are we waiting for that can tim see my comment where this is defined http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: Line 114: DEFINE_int32(kudu_client_timeout_ms, 3 * 60 * 1000, "Timeout (milliseconds) set on Kudu " > Not clear what is timing out. Is it an RPC? Connection attempt? Yes good point. This should apply for all operations, i.e. admin (create/delete tbls), metadata (get tablets), data (scanner and sink i.e. 'KuduSession'). Whether or not there are multiple RPCs is an implementation detail within the operation. I tried to make this more clear. I updated the comment and changed the name. I also simplified the usage where this is set after speaking with the Kudu folks. The BE code actually had some other flags for scanner/session timeouts that I removed and set this one instead. http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: Line 60: Status CreateKuduClient(const std::vector& master_addrs, > remove std:: 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Alex Behm has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: Line 44: DECLARE_int32(kudu_client_timeout_ms); kudu_client_rpc_timeout_ms? or what exactly are we waiting for that can time out? http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: Line 114: DEFINE_int32(kudu_client_timeout_ms, 3 * 60 * 1000, "Timeout (milliseconds) set on Kudu " Not clear what is timing out. Is it an RPC? Connection attempt? Is there a special value for infinity? Is infinity allowed/recommended? http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: Line 60: Status CreateKuduClient(const std::vector& master_addrs, remove std:: http://gerrit.cloudera.org:8080/#/c/4849/4/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 246: // TODO: This is misleading when there are other errors, e.g. timeouts. yup, good point, it's difficult to properly report on many of the Kudu exceptions (connection problem vs. table does not exist, etc.) -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Lars Volker has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 4: Code-Review+1 Thanks. -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 4: I made the changes to address Lars' comments but I'll update the review after I get another review since they're minor and I think someone may have started reviewing what's here already. -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Lars Volker has posted comments on this change. Change subject: IMPALA-3771: Expose kudu client timeout and set default .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: Line 64: b.add_master_server_addr(address); nit: single line? http://gerrit.cloudera.org:8080/#/c/4849/4/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: Line 40: Status CreateKuduClient(const std::vector& master_addrs, Can you add a comment? 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; > I don't think there's a good way to set globals across java and the C++ cod My thought was that we could set this to something like -1 here and then later check that it was actually correctly set/overridden by the backend. This way we would only have the constant stored once. But I think it's fine this way, too. 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: PS2, Line 148: parsedReplicas = Integer.parseInt(replication > This should already be checked and thrown in a nice way by analysis. I thin Ah, didn't know Analysis catches this. I agree that a Precondition makes most sense here. -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Matthew Jacobs has uploaded a new patch set (#3). Change subject: IMPALA-3771: Expose kudu client timeout and set default .. IMPALA-3771: Expose kudu client timeout and set default The Kudu client timeout was too low for Impala usage. This sets the default timeout to 3 minutes and exposes it as a gflag. New timeout tests were added. Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 --- M be/src/catalog/catalog.cc M be/src/common/global-flags.cc M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/exec/kudu-table-sink.cc M be/src/exec/kudu-table-sink.h M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/service/frontend.cc M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test M tests/common/impala_test_suite.py M tests/custom_cluster/test_kudu.py 20 files changed, 197 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/4849/3 -- To view, visit http://gerrit.cloudera.org:8080/4849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew JacobsGerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
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(_), "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 JacobsGerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
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(_), "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 JacobsGerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/4849 Change subject: IMPALA-3771: Expose kudu client timeout and set default .. IMPALA-3771: Expose kudu client timeout and set default The Kudu client timeout was too low for Impala usage. This sets the default timeout to 3 minutes and exposes it as a gflag. New timeout tests were added. Note: Kudu Java client exceptions on timeouts are very verbose and not useful, so the code that catches exceptions around the client APIs that are likely to time out do not set the inner exception otherwise the query errors will be unusable. Instead, the Kudu exceptions are logged and a simple Impala exception is thrown. KUDU-1734 tracks fixing the Kudu errors. After the error messages are fixed, Kudu exceptions can be added back to the Impala exceptions. Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 --- M be/src/catalog/catalog.cc M be/src/common/global-flags.cc M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-table-sink.cc M be/src/service/frontend.cc M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test A testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-impalad.test M tests/common/impala_test_suite.py M tests/custom_cluster/test_kudu.py 16 files changed, 194 insertions(+), 75 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/4849/1 -- To view, visit http://gerrit.cloudera.org:8080/4849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iad95e8e38aad4f76d21bac6879db6c02b3c3e045 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs