kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject [kudu] 02/02: [clock] change in the semantics of TimeService::Init()
Date Sat, 28 Sep 2019 00:44:48 GMT
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 <alexey@apache.org>
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 <aserbin@cloudera.com>
    Reviewed-by: Adar Dembo <adar@cloudera.com>
---
 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.


Mime
View raw message