Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15307 )
Change subject: [tserver] report the newly bootstrapped tablet after OpenTablet completes ...................................................................... Patch Set 13: (7 comments) http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager-test.cc File src/kudu/tserver/ts_tablet_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager-test.cc@162 PS13, Line 162: MonoDelta timeout(MonoDelta::FromSeconds(10)); : MonoTime start(MonoTime::Now()); : while (true) { : GenerateIncrementalTabletReport(&report); : if (report.updated_tablets_size() > 0) { : ASSERT_EQ(report.updated_tablets_size() , 1); : reported_tablet->CopyFrom(report.updated_tablets(0)); : break; : } : MonoDelta elapsed(MonoTime::Now() - start); : ASSERT_TRUE(elapsed < timeout) : << "Waited too long for a tablet marked dirty: " : << elapsed.ToString(); : SleepFor(MonoDelta::FromMilliseconds(10)); : } Is it possible to use ASSERT_EVENTUALLY() instead of custom try-and-wait code? ASSERT_EVENTUALLY() has timeout of 30 seconds, but if 10 seconds is a hard requirement for timeout, consider using AssertEventually([&] {...}, MonoDelta::FromSeconds(10)) http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager-test.cc@421 PS13, Line 421: TEST_F(TsTabletManagerTest, TestTransitionalTabletReport) { I thought it would be possible to have a repro that would show the behavior you captured in a real system, but if this scenario is good enough to make sure the problem will not appear in the wild anymore after the patch, so be it. http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager-test.cc@429 PS13, Line 429: WaitForAReportedTablet Wrap into NO_FATALS() ? http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager-test.cc@430 PS13, Line 430: ASSERT_EQ Here and below: the expected value comes first for ASSERT_EQ/EXPECT_EQ, otherwise it would be hard to comprehend the error message if the assertion triggers. http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager-test.cc@441 PS13, Line 441: WaitForAReportedTablet ditto: wrap into NO_FATALS() http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager.cc@144 PS13, Line 144: tablet_open_inject_latency_for_testing Does it make sense to make this parameter specifying the amount of the delay injected, like tablet_bootstrap_inject_latency_ms does? http://gerrit.cloudera.org:8080/#/c/15307/13/src/kudu/tserver/ts_tablet_manager.cc@1175 PS13, Line 1175: FLAGS_tablet_open_inject_latency_for_testing Consider wrapping this into EXPECT_FALSE() since this if() clause is going to be hit only in case of the new test that you have just added :) -- To view, visit http://gerrit.cloudera.org:8080/15307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I591dc8daceb81f9e800098be77c928d391cdc00a Gerrit-Change-Number: 15307 Gerrit-PatchSet: 13 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Fri, 06 Mar 2020 18:49:39 +0000 Gerrit-HasComments: Yes
