You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2016/10/26 00:57:40 UTC
[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 <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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<std::string>& 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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has uploaded a new patch set (#2).
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-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, 183 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/4849/2
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4849
to look at the new patch set (#6).
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, 209 insertions(+), 101 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/4849/6
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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<std::string>& 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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has uploaded a new patch set (#4).
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/4
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs 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
> #2 is fine with me, as long as there are comments.
Ok, I put a comment in the python test's class header since it is meant to apply to both the impalad and catalogd tests.
--
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Tested-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 <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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<std::string>& 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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3771: Expose kudu client timeout and set default
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3771: Expose kudu client timeout and set default
......................................................................
Patch Set 4:
(2 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?
Done
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<std::string>& master_addrs,
> Can you add a comment?
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 Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes