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
The following commit(s) were added to refs/heads/master by this push:
new e722084 [clock] simplify system time source
e722084 is described below
commit e72208436a625391739217394c67d783e992367a
Author: Alexey Serbin <[email protected]>
AuthorDate: Mon Feb 3 21:37:52 2020 -0800
[clock] simplify system time source
This changelist updates the SystemNtp time provider to use ntp_gettime()
instead of ntp_adjtime() to retrieve current wall-clock time
(both functions are from the NTP kernel API, see [1] for details).
As it turned out, the 'tolerance' field from timex structure is
architecture-dependent constant and never changes: I verified that
for Linux and FreeBSD kernels. So, it's possible to call ntp_adjtime()
in Init() and then use the retrieved value to calculate the clock's
skew upper bound just once.
In addition, this patch enables the 'system' time source for macOS
HighSierra and newer, where the NTP kernel API is supported.
[1] http://man7.org/linux/man-pages/man3/ntp_gettime.3.html
Change-Id: Ib197effb63e8f5e37fd828c88e705112bb7047ce
Reviewed-on: http://gerrit.cloudera.org:8080/15156
Reviewed-by: Adar Dembo <[email protected]>
Tested-by: Kudu Jenkins
---
src/kudu/clock/CMakeLists.txt | 5 ++-
src/kudu/clock/hybrid_clock.cc | 19 ++++++++---
src/kudu/clock/system_ntp.cc | 72 +++++++++++++++++++++++-------------------
src/kudu/clock/system_ntp.h | 16 +++-------
4 files changed, 62 insertions(+), 50 deletions(-)
diff --git a/src/kudu/clock/CMakeLists.txt b/src/kudu/clock/CMakeLists.txt
index c47a15d..190698f 100644
--- a/src/kudu/clock/CMakeLists.txt
+++ b/src/kudu/clock/CMakeLists.txt
@@ -23,8 +23,11 @@ set(CLOCK_SRCS
mock_ntp.cc
system_unsync_time.cc)
-if (NOT APPLE)
+# Darwin 17 (macOS High Sierra, ver. 10.13.6) and higher supports get_ntptime()
+if (NOT APPLE OR ${CMAKE_SYSTEM_VERSION} VERSION_GREATER_EQUAL "17.0.0")
set(CLOCK_SRCS ${CLOCK_SRCS} system_ntp.cc)
+ set_source_files_properties(hybrid_clock.cc
+ PROPERTIES COMPILE_DEFINITIONS "KUDU_HAS_SYSTEM_TIME_SOURCE=1")
endif()
add_library(clock ${CLOCK_SRCS})
diff --git a/src/kudu/clock/hybrid_clock.cc b/src/kudu/clock/hybrid_clock.cc
index 0ea1408..1f80af7 100644
--- a/src/kudu/clock/hybrid_clock.cc
+++ b/src/kudu/clock/hybrid_clock.cc
@@ -59,14 +59,25 @@ TAG_FLAG(use_hybrid_clock, hidden);
// Use the 'system' time source by default in standard (non-test) environment.
// This requires local machine clock to be NTP-synchronized.
-DEFINE_string(time_source, "system",
+DEFINE_string(time_source,
+#if defined(KUDU_HAS_SYSTEM_TIME_SOURCE)
+ "system",
+#else
+ "system_unsync",
+#endif
"The time source that HybridClock should use. Must be one of "
- "'builtin', 'system', 'system_unsync', or 'mock' "
+ "'builtin', "
+#if defined(KUDU_HAS_SYSTEM_TIME_SOURCE)
+ "'system', "
+#endif
+ "'system_unsync', or 'mock' "
"('system_unsync' and 'mock' are for tests only)");
TAG_FLAG(time_source, evolving);
DEFINE_validator(time_source, [](const char* flag_name, const string& value) {
if (boost::iequals(value, "builtin") ||
+#if defined(KUDU_HAS_SYSTEM_TIME_SOURCE)
boost::iequals(value, "system") ||
+#endif
boost::iequals(value, "system_unsync") ||
boost::iequals(value, "mock")) {
return true;
@@ -132,11 +143,9 @@ HybridClock::HybridClock()
Status HybridClock::Init() {
if (boost::iequals(FLAGS_time_source, "mock")) {
time_service_.reset(new clock::MockNtp);
+#if defined(KUDU_HAS_SYSTEM_TIME_SOURCE)
} else if (boost::iequals(FLAGS_time_source, "system")) {
-#ifndef __APPLE__
time_service_.reset(new clock::SystemNtp);
-#else
- time_service_.reset(new clock::SystemUnsyncTime);
#endif
} else if (boost::iequals(FLAGS_time_source, "system_unsync")) {
time_service_.reset(new clock::SystemUnsyncTime);
diff --git a/src/kudu/clock/system_ntp.cc b/src/kudu/clock/system_ntp.cc
index b3a278d..a7c1e6b 100644
--- a/src/kudu/clock/system_ntp.cc
+++ b/src/kudu/clock/system_ntp.cc
@@ -47,41 +47,35 @@ using strings::Substitute;
namespace kudu {
namespace clock {
-const double SystemNtp::kAdjtimexScalingFactor = 65536;
-const uint64_t SystemNtp::kMicrosPerSec = 1000000;
-
namespace {
-// Returns the current time/max error and checks if the clock is synchronized.
-Status CallAdjTime(timex* tx) {
- // Set mode to 0 to query the current time.
- tx->modes = 0;
- int rc = ntp_adjtime(tx);
- if (PREDICT_FALSE(FLAGS_inject_unsync_time_errors)) {
- rc = TIME_ERROR;
- }
+Status NtpStateToStatus(int rc) {
switch (rc) {
case TIME_OK:
return Status::OK();
case -1: // generic error
- // From 'man 2 adjtimex', ntp_adjtime failure implies an improper 'tx'.
- return Status::InvalidArgument("Error reading clock. ntp_adjtime()
failed",
- ErrnoToString(errno));
+ {
+ int err = errno;
+ // From 'man 2 adjtimex', ntp_adjtime failure implies an improper 'tx'.
+ return Status::InvalidArgument(
+ "Error reading clock. ntp_adjtime() failed", ErrnoToString(err));
+ }
case TIME_ERROR:
return Status::ServiceUnavailable(
PREDICT_FALSE(FLAGS_inject_unsync_time_errors) ?
"Injected clock unsync error" :
"Error reading clock. Clock considered unsynchronized");
default:
- // TODO what to do about leap seconds? see KUDU-146
- KLOG_FIRST_N(ERROR, 1) << "Server undergoing leap second. This may cause
consistency issues "
- << "(rc=" << rc << ")";
+ // TODO(dralves): what to do about leap seconds? see KUDU-146
+ KLOG_FIRST_N(ERROR, 1)
+ << "Server undergoing leap second. This may cause consistency issues
"
+ << "(rc=" << rc << ")";
return Status::OK();
}
}
void TryRun(vector<string> cmd, vector<string>* log) {
- string exe, out, err;
+ string exe;
Status s = FindExecutable(cmd[0], {"/sbin", "/usr/sbin/"}, &exe);
if (!s.ok()) {
LOG_STRING(WARNING, log) << "could not find executable: " << cmd[0];
@@ -89,6 +83,8 @@ void TryRun(vector<string> cmd, vector<string>* log) {
}
cmd[0] = exe;
+ string out;
+ string err;
s = Subprocess::Call(cmd, "", &out, &err);
// Subprocess::Call() returns RuntimeError in the case that the process
returns
// a non-zero exit code, but that might still generate useful err.
@@ -102,7 +98,6 @@ void TryRun(vector<string> cmd, vector<string>* log) {
} else {
LOG_STRING(WARNING, log) << "failed to run executable: " << cmd[0];
}
-
}
} // anonymous namespace
@@ -111,22 +106,35 @@ 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.
+Status SystemNtp::Init() {
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;
+ tx.modes = 0; // set mode to 0 for read-only query
+ RETURN_NOT_OK(NtpStateToStatus(ntp_adjtime(&tx)));
+ // The unit of the reported tolerance is ppm with 16-bit fractional part:
+ // 65536 is 1 ppm (see
http://man7.org/linux/man-pages/man3/ntp_adjtime.3.html
+ // for details).
+ skew_ppm_ = tx.tolerance / 65536;
+ return Status::OK();
+}
- if (tx.status & STA_NANO) {
- tx.time.tv_usec /= 1000;
+Status SystemNtp::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) {
+ if (PREDICT_FALSE(FLAGS_inject_unsync_time_errors)) {
+ return NtpStateToStatus(TIME_ERROR);
}
- DCHECK_LE(tx.time.tv_usec, 1e6);
-
- *now_usec = tx.time.tv_sec * kMicrosPerSec + tx.time.tv_usec;
- *error_usec = tx.maxerror;
+ // Read the time. This will return an error if the clock is not synchronized.
+ ntptimeval tv;
+ const int rc = ntp_gettime(&tv);
+ // ntp_gettime() never fails according to its manual page.
+ PCHECK(rc != -1);
+ RETURN_NOT_OK(NtpStateToStatus(rc));
+ uint64_t now = static_cast<uint64_t>(tv.time.tv_sec) * 1000000;
+#ifdef __APPLE__
+ now += tv.time.tv_nsec / 1000;
+#else
+ now += tv.time.tv_usec;
+#endif
+ *now_usec = now;
+ *error_usec = tv.maxerror;
return Status::OK();
}
diff --git a/src/kudu/clock/system_ntp.h b/src/kudu/clock/system_ntp.h
index 1612595..90ec93a 100644
--- a/src/kudu/clock/system_ntp.h
+++ b/src/kudu/clock/system_ntp.h
@@ -16,7 +16,6 @@
// under the License.
#pragma once
-#include <atomic>
#include <cstdint>
#include <string>
#include <vector>
@@ -38,9 +37,7 @@ class SystemNtp : public TimeService {
public:
SystemNtp();
- Status Init() override {
- return Status::OK();
- }
+ Status Init() override;
Status WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) override;
@@ -51,14 +48,9 @@ class SystemNtp : public TimeService {
void DumpDiagnostics(std::vector<std::string>* log) const override;
private:
- // The scaling factor used to obtain ppms. From the adjtimex source:
- // "scale factor used by adjtimex freq param. 1 ppm = 65536"
- static const double kAdjtimexScalingFactor;
-
- static const uint64_t kMicrosPerSec;
-
- // The skew rate in PPM reported by the kernel.
- std::atomic<int64_t> skew_ppm_;
+ // The maximum possible clock frequency skew rate reported by the kernel,
+ // parts-per-million (PPM).
+ int64_t skew_ppm_;
DISALLOW_COPY_AND_ASSIGN(SystemNtp);
};
