Alex Behm has posted comments on this change. Change subject: IMPALA-4134,IMPALA-3704: Kudu INSERT improvements ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/4728/2/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: Line 36: DEFINE_int32(kudu_mutation_buffer_size, 100 * 1024 * 1024, "The size (bytes) of the " > My understanding from the Kudu team is that we always want this enabled, an That's fine, I was mostly concerned about those possible error cases in Open() where we'd abort the query if we fail to set these buffer options in the session_ (and the user has no way of disabling that behavior) Shouldn't we check the value of this config option for sanity, e.g. 0 or negative values or some really low number like 20. Line 128: KUDU_RETURN_IF_ERROR(session_->SetFlushMode( > I don't think these configuration operations should fail unless we called t Why not DCHECK if we are sure it's our bug? Or is it possible that this call fails due to session timeout or similar? http://gerrit.cloudera.org:8080/#/c/4728/3/be/src/exec/kudu-table-sink.h File be/src/exec/kudu-table-sink.h: Line 35: /// The data is added to a Kudu in Send(). The Kudu client is configured to automatically typo: remove first 'a' Line 37: /// requires specifying a mutation buffer size and a buffer flush watermark percentage. ... in the Kudu client. Line 42: /// that results in Kudu flushing after 7MB is in a particular buffer (of the 100MB of is in a buffer for a particular destination? http://gerrit.cloudera.org:8080/#/c/4728/3/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: Line 323: ==== Don't we need to also modify the expected ERRORS of existing tests for KUDU_KEY_ALREADY_PRESENT and KUDU_KEY_NOT_FOUND Or do we not have tests for that? -- To view, visit http://gerrit.cloudera.org:8080/4728 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5542b9a061b01c543a139e8722560b1365f06595 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
