Matthew Jacobs has posted comments on this change. Change subject: WIP: Use AUTO_FLUSH_BACKGROUND for Kudu sink ......................................................................
Patch Set 1: (4 comments) Thanks, Todd. Can you ref IMPALA-4134 in the commit comment? http://gerrit.cloudera.org:8080/#/c/4670/1/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: PS1, Line 176: SetString I assume this doesn't result in any more copying than we had before? PS1, Line 225: // TODO: instead of adding them here and subtracting errors in CheckForErrors, : // would it be better to _set_ the value to the total number of written rows : // minus the errors minus the current buffer size? Yeah, this is a bit weird as it is. I'd vote to compute this as you described in CheckForErrors even though that method name doesn't really hint it'll update this counter, but a brief mention in the header comment should be OK. I don't actually us use this value for non-HDFS tables besides something that looks like legacy code in impala-beeswax-server. PS1, Line 234: if (session_->CountPendingErrors() == 0) { : return Status::OK(); : } nit: 1 line PS1, Line 281: // TODO: these counters don't make so much sense anymore. : SCOPED_TIMER(kudu_flush_timer_); : COUNTER_ADD(kudu_flush_counter_, 1); can you remove them then? -- To view, visit http://gerrit.cloudera.org:8080/4670 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665ce82d1caf64cf5bdf4bc0e15edbf9f1dc5ba0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-HasComments: Yes
