Mike Percy has posted comments on this change. Change subject: KUDU-1785. Fix potential crash in TabletCopySourceSession ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/5363/2/src/kudu/integration-tests/tablet_copy_client_session-itest.cc File src/kudu/integration-tests/tablet_copy_client_session-itest.cc: PS2, Line 70: while (workload.rows_inserted() < 10000) { : SleepFor(MonoDelta::FromMilliseconds(10)); : } > I've never really understood this; every tablet copy has something to copy, Personally, I like this because: 1) actually writing data is a good verification that the tablet is generally in working condition and 2) it's nice to have some actual data in the WAL to copy since copying blocks and WALs require tablet copy round-trips. http://gerrit.cloudera.org:8080/#/c/5363/4/src/kudu/integration-tests/tablet_copy_client_session-itest.cc File src/kudu/integration-tests/tablet_copy_client_session-itest.cc: Line 44: > Nit: num_tablets defaults to 1 ? Let's change it later if it seems super annoying. I don't think this is very important. Line 46: }; > Nit: Do we want to move the timeout to individual tests since future tests In my opinion, let's not worry about it in the spirit of YAGNI. PS4, Line 121: > Do we want to wait for tablet copy or just wait till we get a Status::Illeg This test fails reliably for me. I'll loop it to double check. PS4, Line 122: > Why is this necessary for testing this fix ? I'm just trying to return the system to the state it was in when entering the loop. -- To view, visit http://gerrit.cloudera.org:8080/5363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f3ec35885dbf1a81c23ac10b1c9556dfddbd4b7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
