Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8464 )
Change subject: IMPALA-4591: Bound Kudu client error mem usage ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8464/2/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/8464/2/be/src/exec/kudu-table-sink.cc@350 PS2, Line 350: mem_tracker_->Release(client_tracked_bytes_); > The mutation buffer is cleared by KuduSession::Flush() and the error buffer When you say "cleared" do you mean zeroed or freed? Also, if CountPendingErrors() is 0, we skip GetPendingErrors() -- is it possible for the client to have a non-zero sized buffer still in that case? FlushFinal() isn't guaranteed to be called if fragment instance execution was terminated due to an error (see FragmentInstanceState::ExecInternal()). The session_ is a shared_ptr -- I wonder if the implicit client API is that we should be nulling it out to drop our reference to cause the session resources to be freed? The client.h header does say this: /// @return A new session object; caller is responsible for destroying it. sp::shared_ptr<KuduSession> NewSession(); http://gerrit.cloudera.org:8080/#/c/8464/2/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/8464/2/tests/custom_cluster/test_kudu.py@78 PS2, Line 78: assert "Error overflow in Kudu session." in str(e) > Yes, there are existing tests in kudu_insert.test that generate thousands o But what about when the limit is set to 1024? Is there a way to determine that we don't hit the limit too early? Anyway, you can decide whether the testing in that regard is sufficient already or not. -- 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: 2 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 17 Nov 2017 23:14:14 +0000 Gerrit-HasComments: Yes
