Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14986 )
Change subject: KUDU-3011 p3: mechanism to quiesce scans ...................................................................... Patch Set 1: (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::QUIESCING: // fall-through Equivalent change needed in the Java client? I see two instances of TABLET_NOT_RUNNING that may be relevant. 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 would it be clearer if the FT-ness of the scans were elided altogether, given that the default scan behavior is non-FT? http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@212 PS1, Line 212: // Restarting the quiesced tablet server shouldn't affect the ongoing read : // workload, since there are no scans on that server. Would a stronger invariant be "stopping the quiesced TS shouldn't affect the ongoing workload"? Why bother restarting it? Same in other new tests. http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@223 PS1, Line 223: FLAGS_raft_heartbeat_interval_ms = 100; Why this? http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@240 PS1, Line 240: vector<MiniTabletServer*> ts_with_scans; Since the test only requires one TS to have an active scan, why bother populating an entire vector? Could just find the first such TS? http://gerrit.cloudera.org:8080/#/c/14986/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@348 PS1, Line 348: // Set a tiny batch size to encourage many batches for a single scan. This : // will emulate long-running scans. : FLAGS_scanner_default_batch_size_bytes = 1; Probably doesn't matter for this test? 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: I Nit: add a space 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 CheckTabletServerQuiescing(const TabletServer* server, RespClass* resp, Add a OrRespond suffix? 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: QUIESCING = 22; Do you think we could/should get by reusing TABLET_NOT_RUNNING for this case? -- 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: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jan 2020 00:03:17 +0000 Gerrit-HasComments: Yes
