Todd Lipcon has posted comments on this change.

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/integration-tests/external_mini_cluster-test.cc
File src/kudu/integration-tests/external_mini_cluster-test.cc:

Line 128: // any crashes.
might make more sense to put this in a different test case... this test in 
theory is supposed to be just testing the test infra, not actual 
functionality/bugs


Line 142:       MonoDelta::FromMicroseconds(100));
maybe you'd hit the race more with a super low value like 1us?

maybe even start multiple threads if you really wanna make a solid stress test 
of this?


Line 154:   work.StopAndJoin();
might be more fruitful if you set the log rolling threshold to be really low, 
and inserted much more data, since the particular MRS-related test is a 
per-segment thing.


Line 156:   // Restart the cluster. Nothing should crash.
how about an AssertNoCrashes before the restart, too?


http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:

Line 237:     LOG(FATAL) <<
should we consider DFATAL here? would it actually cause incorrect behavior to 
hit this in a real cluster?


-- 
To view, visit http://gerrit.cloudera.org:8080/4394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to