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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
