Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14986 )
Change subject: KUDU-3011 p3: mechanism to quiesce scans ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/client/scanner-internal.cc@308 PS1, Line 308: case tserver::TabletServerErrorPB::TABLET_NOT_RUNNING: > Equivalent change needed in the Java client? I see two instances of TABLET_ Ack http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc File src/kudu/integration-tests/tablet_server_quiescing-itest.cc: PS1: > Highlighting the "non FT" nature of these scans is a useful detail, but wou Oops, didn't mean to swap the default. I'm calling it out because it requires explicit setting for TestWorkloads, even if it's not the default for scans in general. http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@212 PS1, Line 212: : // Stopping the quiesced tablet server shouldn't affe > Would a stronger invariant be "stopping the quiesced TS shouldn't affect th Done http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@223 PS1, Line 223: // This test will change leaders frequently, so set a low Raft heartbeat. > Why this? Done http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@240 PS1, Line 240: // Wait for the scans to begin. > Since the test only requires one TS to have an active scan, why bother popu Done http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@348 PS1, Line 348: string table_name; : { : auto rw_workload = CreateFaultIntolerantR > Probably doesn't matter for this test? Updated the comment. http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/test_workload.cc File src/kudu/integration-tests/test_workload.cc: http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/test_workload.cc@233 PS1, Line 233: > Nit: add a space Done http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/tserver/tablet_service.cc@362 PS1, Line 362: bool CheckTabletServerQuiescingOrRespond(const TabletServer* server, RespClass* resp, > Add a OrRespond suffix? Done http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/tserver/tserver.proto@109 PS1, Line 109: required Code code = 1 [ default = UNKNOWN_ERROR ]; > Do you think we could/should get by reusing TABLET_NOT_RUNNING for this cas Yeah, I actually did that first and then added the new state. But good point about the Java client -- makes me want to reuse it more, especially since it's not really clear that there are real differences between the enums at the moment. -- To view, visit http://gerrit.cloudera.org:8080/14986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idedca29f40b0a8576245be0f7cfad2be29db4135 Gerrit-Change-Number: 14986 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jan 2020 01:01:49 +0000 Gerrit-HasComments: Yes
