This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 7a347a7792a0dd317aece63f8a9d6aee0b2b680d Author: Alexey Serbin <[email protected]> AuthorDate: Tue Oct 27 18:22:11 2020 -0700 [tserver] --scanner_max_wait_ms is a run-time flag This patch marks --scanner_max_wait_ms flag with the 'runtime' tag to reflect its de facto behavior. I also did a minor clean-up of the related code. Change-Id: I5907571db5de76446c5ce04ca64da8e95328087d Reviewed-on: http://gerrit.cloudera.org:8080/16669 Tested-by: Alexey Serbin <[email protected]> Reviewed-by: Andrew Wong <[email protected]> --- src/kudu/consensus/time_manager.cc | 1 - src/kudu/tserver/tablet_service.cc | 60 +++++++++++++++++++++----------------- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/kudu/consensus/time_manager.cc b/src/kudu/consensus/time_manager.cc index ad421cd..03418c6 100644 --- a/src/kudu/consensus/time_manager.cc +++ b/src/kudu/consensus/time_manager.cc @@ -51,7 +51,6 @@ DEFINE_int32(safe_time_max_lag_ms, 30 * 1000, TAG_FLAG(safe_time_max_lag_ms, experimental); DECLARE_int32(raft_heartbeat_interval_ms); -DECLARE_int32(scanner_max_wait_ms); using kudu::clock::Clock; using std::string; diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc index 38f5386..6b0704a 100644 --- a/src/kudu/tserver/tablet_service.cc +++ b/src/kudu/tserver/tablet_service.cc @@ -136,10 +136,12 @@ DEFINE_bool(scanner_allow_snapshot_scans_with_logical_timestamps, false, TAG_FLAG(scanner_allow_snapshot_scans_with_logical_timestamps, unsafe); DEFINE_int32(scanner_max_wait_ms, 1000, - "The maximum amount of time (in milliseconds) we'll hang a scanner thread waiting for " - "safe time to advance or ops to commit, even if its deadline allows waiting " - "longer."); + "The maximum amount of time (in milliseconds) a scan " + "at a snapshot is allowed to wait for safe time to advance " + "or pending write operations to apply, even if the deadline " + "of the scan request itself allows for waiting longer."); TAG_FLAG(scanner_max_wait_ms, advanced); +TAG_FLAG(scanner_max_wait_ms, runtime); // Fault injection flags. DEFINE_int32(scanner_inject_latency_on_each_batch_ms, 0, @@ -3050,7 +3052,7 @@ Status TabletServiceImpl::HandleContinueScanRequest(const ScanRequestPB* req, namespace { // Helper to clamp a client deadline for a scan to the max supported by the server. MonoTime ClampScanDeadlineForWait(const MonoTime& deadline, bool* was_clamped) { - MonoTime now = MonoTime::Now(); + const MonoTime now = MonoTime::Now(); if ((deadline - now).ToMilliseconds() > FLAGS_scanner_max_wait_ms) { *was_clamped = true; return now + MonoDelta::FromMilliseconds(FLAGS_scanner_max_wait_ms); @@ -3069,12 +3071,30 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb, boost::optional<Timestamp>* snap_start_timestamp, Timestamp* snap_timestamp, TabletServerErrorPB::Code* error_code) { - switch (scan_pb.read_mode()) { + const auto read_mode = scan_pb.read_mode(); + switch (read_mode) { case READ_AT_SNAPSHOT: // Fallthrough intended case READ_YOUR_WRITES: break; default: - LOG(FATAL) << "Unsupported snapshot scan mode specified."; + LOG(FATAL) << Substitute("$0: unsupported snapshot scan mode", read_mode); + } + + // Validate other input parameters as well in the very beginning. + if (scan_pb.has_snap_start_timestamp()) { + if (read_mode != READ_AT_SNAPSHOT) { + // TODO(mpercy): Should we allow READ_YOUR_WRITES mode? There is no + // obvious use for it, but also no obvious reason not to support it, + // except for the fact that we would also have to test it. + *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC; + return Status::InvalidArgument("scan start timestamp is only supported " + "in READ_AT_SNAPSHOT read mode"); + } + if (scan_pb.order_mode() != ORDERED) { + *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC; + return Status::InvalidArgument("scan start timestamp is only supported " + "in ORDERED order mode"); + } } // Based on the read mode, pick a timestamp and verify it. @@ -3086,7 +3106,8 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb, } // Reduce the client's deadline by a few msecs to allow for overhead. - MonoTime client_deadline = rpc_context->GetClientDeadline() - MonoDelta::FromMilliseconds(10); + const MonoTime client_deadline = + rpc_context->GetClientDeadline() - MonoDelta::FromMilliseconds(10); // Its not good for the tablet server or for the client if we hang here forever. The tablet // server will have one less available thread and the client might be stuck spending all @@ -3103,7 +3124,7 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb, // snapshot to be clean below, allows us to always return the same data when scanning at // the same timestamp (repeatable reads). TRACE("Waiting safe time to advance"); - MonoTime before = MonoTime::Now(); + const MonoTime before = MonoTime::Now(); s = time_manager->WaitUntilSafe(tmp_snap_timestamp, final_deadline); tablet::MvccSnapshot snap; @@ -3114,16 +3135,16 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb, tmp_snap_timestamp, &snap, client_deadline); } - // If we got an TimeOut but we had clamped the deadline, return a ServiceUnavailable instead - // so that the client retries. - if (PREDICT_FALSE(s.IsTimedOut() && was_clamped)) { + // If we got an TimeOut but we had clamped the deadline, return + // ServiceUnavailable to make the client retry the scan operation soon. + if (s.IsTimedOut() && was_clamped) { *error_code = TabletServerErrorPB::THROTTLED; return Status::ServiceUnavailable(s.CloneAndPrepend( "could not wait for desired snapshot timestamp to be consistent").ToString()); } RETURN_NOT_OK(s); - uint64_t duration_usec = (MonoTime::Now() - before).ToMicroseconds(); + const auto duration_usec = (MonoTime::Now() - before).ToMicroseconds(); tablet->metrics()->snapshot_read_inflight_wait_duration->Increment(duration_usec); TRACE("All operations in snapshot committed. Waited for $0 microseconds", duration_usec); @@ -3134,19 +3155,6 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb, boost::optional<Timestamp> tmp_snap_start_timestamp; if (scan_pb.has_snap_start_timestamp()) { - if (scan_pb.read_mode() != READ_AT_SNAPSHOT) { - // TODO(mpercy): Should we allow READ_YOUR_WRITES mode? There is no - // obvious use for it, but also no obvious reason not to support it, - // except for the fact that we would also have to test it. - *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC; - return Status::InvalidArgument("scan start timestamp is only supported " - "in READ_AT_SNAPSHOT read mode"); - } - if (scan_pb.order_mode() != ORDERED) { - *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC; - return Status::InvalidArgument("scan start timestamp is only supported " - "in ORDERED order mode"); - } tmp_snap_start_timestamp = Timestamp(scan_pb.snap_start_timestamp()); opts.snap_to_exclude = MvccSnapshot(*tmp_snap_start_timestamp); opts.include_deleted_rows = true; @@ -3157,7 +3165,7 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb, // iterators are open but there is no point in doing the work to initialize // the iterators and spending the time to wait for a snapshot timestamp to be // readable when we can't read back to one of the requested timestamps. - RETURN_NOT_OK_EVAL(VerifyLegalSnapshotTimestamps(tablet, scan_pb.read_mode(), + RETURN_NOT_OK_EVAL(VerifyLegalSnapshotTimestamps(tablet, read_mode, tmp_snap_start_timestamp, tmp_snap_timestamp), *error_code = TabletServerErrorPB::INVALID_SNAPSHOT);
