Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout are separate entities ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@913 PS5, Line 913: FLAGS_master_inject_latency_on_tablet_lookups_ms = : rpcTimeoutMs * 2; // tablet lookup will trigger timeout style nit for here and elsewhere with in-line comments: another way to format this might be // This is to make tablet lookups timing out. FLAGS_master_inject_latency_on_tablet_lookups_ms = rpcTimeoutMs * 2; That way it's less line breaks within a single semantic scope/statement and more space for explanations. http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@930 PS5, Line 930: auto ent = cluster_->mini_master()->master()->metric_entity(); Shouldn't we check the metric value of GetTableLocations before issuing session->Apply call? Since the all the rest if a black box from this point, the preliminary code might do some calls in background (it's not so as of now, but the code in the test should be more explicit in that regard). http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@932 PS5, Line 932: ASSERT_EQ(METRIC_handler_latency_kudu_master_MasterService_GetTableLocations.Instantiate(ent) It would be great to add a comment to explain why seeing that number this number of requests guarantees the timeouts for session and an RPC are different. It's not very obvious for a reader who has no enough context. Alternatively, you could add an explanation of the overall scenario just above the TEST_F line that introduces this scenario. http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@932 PS5, Line 932: ASSERT_EQ(METRIC_handler_latency_kudu_master_MasterService_GetTableLocations.Instantiate(ent) : ->TotalCount(), : 2); // check that there was a retry nit: the expected value comes first in {ASSERT,EXPECT}_EQ() macros, otherwise it's hard to comprehend the output if the assertion triggers http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@935 PS5, Line 935: set_timeout_back_to_zero.join(); What if one of the assertions above trigger? Will the test crash because of destroying thread instance while it's not yet joined? To fix this issue, consider using ScopedCleanup or the SCOPED_CLEANUP() macro. See https://github.com/apache/kudu/blob/10efaf2c77dfe5e4474505e0267c583c011703be/src/kudu/clock/hybrid_clock-test.cc#L326-L331 for an example. http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@936 PS5, Line 936: } BTW, checking for the exact number of requests might be a source of flakiness due to scheduler anomalies. Instead, I'd recommend checking that the number of registered requests after issuing Apply() is greater than the number of registered requests after Apply() succeeded. -- To view, visit http://gerrit.cloudera.org:8080/19175 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036 Gerrit-Change-Number: 19175 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 16 Nov 2022 21:32:09 +0000 Gerrit-HasComments: Yes
