Adar Dembo 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 Will do. Line 142: MonoDelta::FromMicroseconds(100)); > maybe you'd hit the race more with a super low value like 1us? At first I tried 1us and 10 checker threads, but there was too much load and cluster.Restart() timed out in 20% of runs. So I raised the poll period to 1ms, and now 23/1000 runs fail with the MemTracker error, but I still can't reliably repro it locally. Do you think that's good enough? Line 154: work.StopAndJoin(); > might be more fruitful if you set the log rolling threshold to be really lo AFAICT the number of log segments is orthogonal to the number of times tablet::RewindTabletForBootstrap is called (which is just once). Line 156: // Restart the cluster. Nothing should crash. > how about an AssertNoCrashes before the restart, too? Sure. 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 It would lead to incorrect memory accounting, I think, but maybe that's the worst of it. I'll change to DFATAL and return the first found tracker. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
