Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8464 )
Change subject: IMPALA-4591: Bound Kudu client error mem usage ...................................................................... Patch Set 1: (8 comments) This is a nice improvement. Had some questions/ideas but as-is this seems strictly better than before. http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.h File be/src/exec/kudu-table-sink.h: http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.h@105 PS1, Line 105: allocated nit: "consumed" to be consistent with the memtracker terminology. http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.h@106 PS1, Line 106: allocated_mem_ Maybe client_tracked_bytes_ to make it clearer that the unit is bytes and it's tracked for accounting purposes. http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.cc@a52 PS1, Line 52: Was this flag documented? Just wondering if we should consider what happens if someone set this manually. http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.cc@39 PS1, Line 39: DEFINE_int32(kudu_mutation_buffer_size, DEFAULT_KUDU_MUTATION_BUFFER_SIZE, Just throwing out ideas here, but did we think about pros/cons of making these query options? I guess mostly these defaults should be fine but sometimes it turns out to be inconvenient to only have a global setting (and to have to restart Impala for it to take effect.) http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.cc@124 PS1, Line 124: FLAGS_kudu_error_buffer_size > 1024 ? FLAGS_kudu_error_buffer_size : 1024; Is this equivalent to the following? max<int64_t>(1024, FLAGS_kudu_error_buffer_size) http://gerrit.cloudera.org:8080/#/c/8464/1/be/src/exec/kudu-table-sink.cc@132 PS1, Line 132: RETURN_IF_ERROR(state->exec_env()->GetKuduClient(table_desc_->kudu_master_addresses(), &client_)); nit: long line. http://gerrit.cloudera.org:8080/#/c/8464/1/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/8464/1/tests/custom_cluster/test_kudu.py@66 PS1, Line 66: @CustomClusterTestSuite.with_args(impalad_args="-kudu_error_buffer_size=1024") It might be faster to make this a regular query test but insert more data so that it exceeds the default error buffer limit. Starting up a new cluster takes a long time (to be honest, this is fine though). http://gerrit.cloudera.org:8080/#/c/8464/1/tests/custom_cluster/test_kudu.py@74 PS1, Line 74: prsent present -- To view, visit http://gerrit.cloudera.org:8080/8464 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I186ddb3f3b5865e08f17dba57cf6640591d06b14 Gerrit-Change-Number: 8464 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 03 Nov 2017 23:41:13 +0000 Gerrit-HasComments: Yes
