impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4591: Bound Kudu client error mem usage
Date Fri, 17 Nov 2017 23:14:14 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Nov 2017 23:14:14 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message