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 fcbca3dd1234a2c0a41b6dcf9f1d7b6410f33c34 Author: Alexey Serbin <[email protected]> AuthorDate: Fri Sep 27 12:39:27 2019 -0700 [clock] change in the semantics of TimeService::Init() This patch introduces a change in the semantics of the TimeService's initialization method Init(). With this patch, TimeService::Init() no longer waits for the clock synchronisation to happen for NTP-based time services. Users of the TimeService interface should rely on TimeService::WalltimeWithError() to get the status of the clock synchronisation after calling TimeService::Init(). This is a follow-up to e110cb88b145edb6d536ae0806e02736f5bf51e5. Change-Id: Ie7245b2cd2c3001a508235f9e539d7202d3ca994 Reviewed-on: http://gerrit.cloudera.org:8080/14317 Tested-by: Alexey Serbin <[email protected]> Reviewed-by: Adar Dembo <[email protected]> --- src/kudu/clock/hybrid_clock.cc | 12 +++++++-- src/kudu/clock/hybrid_clock.h | 4 +-- src/kudu/clock/mock_ntp.h | 1 - src/kudu/clock/system_ntp.cc | 55 ++++++++++++++++++------------------------ src/kudu/clock/system_ntp.h | 11 +++++---- src/kudu/clock/time_service.h | 18 ++++++++------ 6 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/kudu/clock/hybrid_clock.cc b/src/kudu/clock/hybrid_clock.cc index 6db1990..0163271 100644 --- a/src/kudu/clock/hybrid_clock.cc +++ b/src/kudu/clock/hybrid_clock.cc @@ -134,13 +134,19 @@ Status HybridClock::Init() { } else { return Status::InvalidArgument("invalid NTP source", FLAGS_time_source); } + RETURN_NOT_OK(time_service_->Init()); + // Make sure the underlying clock service is available (e.g., for NTP-based + // clock make sure it's synchronized with its NTP source). If requested, wait + // up to the specified timeout for the clock to become ready to use. const auto wait_s = FLAGS_ntp_initial_sync_wait_secs; const auto deadline = MonoTime::Now() + MonoDelta::FromSeconds(wait_s); bool need_log = true; Status s; + uint64_t now_usec; + uint64_t error_usec; do { - s = time_service_->Init(); + s = time_service_->WalltimeWithError(&now_usec, &error_usec); if (!s.IsServiceUnavailable()) { break; } @@ -164,8 +170,10 @@ Status HybridClock::Init() { return s.CloneAndPrepend("timed out waiting for clock synchronisation"); } + LOG(INFO) << Substitute("HybridClock initialized: " + "now $0 us; error $1 us; skew $2 ppm", + now_usec, error_usec, time_service_->skew_ppm()); state_ = kInitialized; - return Status::OK(); } diff --git a/src/kudu/clock/hybrid_clock.h b/src/kudu/clock/hybrid_clock.h index ed23696..db4cfef 100644 --- a/src/kudu/clock/hybrid_clock.h +++ b/src/kudu/clock/hybrid_clock.h @@ -158,13 +158,11 @@ class HybridClock : public Clock { } private: - friend class TestNtp; - // Obtains the current wallclock time and maximum error in microseconds, // and checks if the clock is synchronized. // // On OS X, the error will always be 0. - kudu::Status WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec); + Status WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec); // Same as above, but exits with a FATAL if there is an error. void WalltimeWithErrorOrDie(uint64_t* now_usec, uint64_t* error_usec); diff --git a/src/kudu/clock/mock_ntp.h b/src/kudu/clock/mock_ntp.h index 94d4a06..2f71295 100644 --- a/src/kudu/clock/mock_ntp.h +++ b/src/kudu/clock/mock_ntp.h @@ -32,7 +32,6 @@ namespace clock { class MockNtp : public TimeService { public: MockNtp() = default; - virtual ~MockNtp() = default; Status Init() override { return Status::OK(); diff --git a/src/kudu/clock/system_ntp.cc b/src/kudu/clock/system_ntp.cc index 9163175..b3a278d 100644 --- a/src/kudu/clock/system_ntp.cc +++ b/src/kudu/clock/system_ntp.cc @@ -21,6 +21,7 @@ #include <sys/timex.h> #include <cerrno> +#include <limits> #include <ostream> #include <string> #include <vector> @@ -106,6 +107,29 @@ void TryRun(vector<string> cmd, vector<string>* log) { } // anonymous namespace +SystemNtp::SystemNtp() + : skew_ppm_(std::numeric_limits<int64_t>::max()) { +} + +Status SystemNtp::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) { + // Read the time. This will return an error if the clock is not synchronized. + timex tx; + RETURN_NOT_OK(CallAdjTime(&tx)); + + // Calculate the sleep skew adjustment according to the max tolerance of the clock. + // Tolerance comes in parts per million but needs to be applied a scaling factor. + skew_ppm_ = tx.tolerance / kAdjtimexScalingFactor; + + if (tx.status & STA_NANO) { + tx.time.tv_usec /= 1000; + } + DCHECK_LE(tx.time.tv_usec, 1e6); + + *now_usec = tx.time.tv_sec * kMicrosPerSec + tx.time.tv_usec; + *error_usec = tx.maxerror; + return Status::OK(); +} + void SystemNtp::DumpDiagnostics(vector<string>* log) const { LOG_STRING(ERROR, log) << "Dumping NTP diagnostics"; TryRun({"ntptime"}, log); @@ -131,36 +155,5 @@ void SystemNtp::DumpDiagnostics(vector<string>* log) const { TryRun({"chronyc", "-n", "sources"}, log); } -Status SystemNtp::Init() { - timex timex; - RETURN_NOT_OK(CallAdjTime(&timex)); - - // Calculate the sleep skew adjustment according to the max tolerance of the clock. - // Tolerance comes in parts per million but needs to be applied a scaling factor. - skew_ppm_ = timex.tolerance / kAdjtimexScalingFactor; - - LOG(INFO) << "NTP initialized." - << " Skew: " << skew_ppm_ << "ppm" - << " Current error: " << timex.maxerror << "us"; - - return Status::OK(); -} - -Status SystemNtp::WalltimeWithError(uint64_t *now_usec, - uint64_t *error_usec) { - // Read the time. This will return an error if the clock is not synchronized. - timex tx; - RETURN_NOT_OK(CallAdjTime(&tx)); - - if (tx.status & STA_NANO) { - tx.time.tv_usec /= 1000; - } - DCHECK_LE(tx.time.tv_usec, 1e6); - - *now_usec = tx.time.tv_sec * kMicrosPerSec + tx.time.tv_usec; - *error_usec = tx.maxerror; - return Status::OK(); -} - } // namespace clock } // namespace kudu diff --git a/src/kudu/clock/system_ntp.h b/src/kudu/clock/system_ntp.h index 5fd8fe7..3b3a723 100644 --- a/src/kudu/clock/system_ntp.h +++ b/src/kudu/clock/system_ntp.h @@ -16,6 +16,7 @@ // under the License. #pragma once +#include <atomic> #include <cstdint> #include <string> #include <vector> @@ -35,11 +36,11 @@ namespace clock { // to keep the kernel's timekeeping up to date and in sync. class SystemNtp : public TimeService { public: - SystemNtp() = default; + SystemNtp(); - // Ensure that the kernel's timekeeping status indicates that it is currently - // in sync, and initialize various internal parameters. - virtual Status Init() override; + virtual Status Init() override { + return Status::OK(); + } virtual Status WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) override; @@ -57,7 +58,7 @@ class SystemNtp : public TimeService { static const uint64_t kMicrosPerSec; // The skew rate in PPM reported by the kernel. - uint64_t skew_ppm_ = 0; + std::atomic<int64_t> skew_ppm_; DISALLOW_COPY_AND_ASSIGN(SystemNtp); }; diff --git a/src/kudu/clock/time_service.h b/src/kudu/clock/time_service.h index cc28714..6851bbf 100644 --- a/src/kudu/clock/time_service.h +++ b/src/kudu/clock/time_service.h @@ -32,19 +32,21 @@ class TimeService { TimeService() = default; virtual ~TimeService() = default; - // Initialize the NTP source, validating that it is available and properly - // synchronized: Status::OK() is returned in such case. If the source - // is not yet synchronized, then Status::ServiceUnavailable() is returned: - // a caller may call this method again to eventually get Status::OK(). - // In case of other non-OK() return statuses, the caller should not invoke - // this method again. + // Initialize the clock source. No verification is performed on the + // synchronisation status of the clock. To verify the clock is synchronized + // and the time service is ready to use, make sure WalltimeWithError() returns + // Status::OK() after calling Init(). The Init() method it itself should be + // called only once. virtual Status Init() = 0; // Return the current wall time in microseconds since the Unix epoch in '*now_usec'. // The current maximum error bound in microseconds is returned in '*error_usec'. + // Neither of the output parameters can be null. // - // May return a bad Status if the NTP service has become unsynchronized or otherwise - // unavailable. + // May return a bad Status if the NTP service has become unsynchronized or + // otherwise unavailable. In case if the method returns ServiceUnavailable() + // after calling Init(), the caller may call this method again and eventually + // get Status::OK() if anticipating the clock synchronisation to happen soon. virtual Status WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) = 0; // Return the estimated max amount of clock skew as configured by this NTP service.
