You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/10/24 22:21:01 UTC
[2/4] incubator-impala git commit: IMPALA-5599: Clean up references
to TimestampValue in be/src.
IMPALA-5599: Clean up references to TimestampValue in be/src.
This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.
TimestampValue::{LocalTime(), UtcTime()} have been retired.
BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.
Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Reviewed-on: http://gerrit.cloudera.org:8080/8305
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/1640aa97
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/1640aa97
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/1640aa97
Branch: refs/heads/master
Commit: 1640aa9796879405c7112de3065c7fbe88064e8a
Parents: 31c440b
Author: Zoram Thanga <zo...@cloudera.com>
Authored: Tue Oct 17 15:51:26 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Tue Oct 24 21:29:49 2017 +0000
----------------------------------------------------------------------
be/src/exprs/expr-test.cc | 17 +++++++++-----
be/src/runtime/timestamp-value.h | 20 ++++------------
be/src/runtime/timestamp-value.inline.h | 8 +++++++
be/src/service/client-request-state.cc | 8 +++----
be/src/service/client-request-state.h | 8 +++----
be/src/service/impala-http-handler.cc | 31 +++++++++----------------
be/src/service/impala-server.cc | 18 ++++++--------
be/src/service/impala-server.h | 4 ++--
be/src/statestore/statestore-subscriber.cc | 2 +-
be/src/util/common-metrics.cc | 5 ++--
be/src/util/time.h | 5 ++++
11 files changed, 59 insertions(+), 67 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/exprs/expr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 8fa252b..ed78535 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -51,6 +51,7 @@
#include "runtime/string-value.h"
#include "runtime/timestamp-parse-util.h"
#include "runtime/timestamp-value.h"
+#include "runtime/timestamp-value.inline.h"
#include "service/fe-support.h"
#include "service/impala-server.h"
#include "testutil/impalad-query-executor.h"
@@ -5626,16 +5627,20 @@ TEST_F(ExprTest, TimestampFunctions) {
// Test that the other current time functions are also reasonable.
TimestampValue timestamp_result;
- TimestampValue start_time = TimestampValue::LocalTime();
+ TimestampValue start_time = TimestampValue::FromUnixTimeMicros(UnixMicros());
timestamp_result = ConvertValue<TimestampValue>(GetValue("now()", TYPE_TIMESTAMP));
- EXPECT_BETWEEN(start_time, timestamp_result, TimestampValue::LocalTime());
+ EXPECT_BETWEEN(start_time, timestamp_result,
+ TimestampValue::FromUnixTimeMicros(UnixMicros()));
timestamp_result = ConvertValue<TimestampValue>(GetValue("current_timestamp()",
TYPE_TIMESTAMP));
- EXPECT_BETWEEN(start_time, timestamp_result, TimestampValue::LocalTime());
- const TimestampValue utc_start_time = TimestampValue::UtcTime();
+ EXPECT_BETWEEN(start_time, timestamp_result,
+ TimestampValue::FromUnixTimeMicros(UnixMicros()));
+ const TimestampValue utc_start_time =
+ TimestampValue::UtcFromUnixTimeMicros(UnixMicros());
timestamp_result = ConvertValue<TimestampValue>(GetValue("utc_timestamp()",
TYPE_TIMESTAMP));
- EXPECT_BETWEEN(utc_start_time, timestamp_result, TimestampValue::UtcTime());
+ EXPECT_BETWEEN(utc_start_time, timestamp_result,
+ TimestampValue::UtcFromUnixTimeMicros(UnixMicros()));
// UNIX_TIMESTAMP() has second precision so the comparison start time is shifted back
// a second to ensure an earlier value.
unix_start_time =
@@ -5643,7 +5648,7 @@ TEST_F(ExprTest, TimestampFunctions) {
timestamp_result = ConvertValue<TimestampValue>(GetValue(
"cast(unix_timestamp() as timestamp)", TYPE_TIMESTAMP));
EXPECT_BETWEEN(TimestampValue::FromUnixTime(unix_start_time - 1), timestamp_result,
- TimestampValue::LocalTime());
+ TimestampValue::FromUnixTimeMicros(UnixMicros()));
// Test that UTC and local time represent the same point in time
{
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/runtime/timestamp-value.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/timestamp-value.h b/be/src/runtime/timestamp-value.h
index 0ad7540..556225b 100644
--- a/be/src/runtime/timestamp-value.h
+++ b/be/src/runtime/timestamp-value.h
@@ -106,6 +106,10 @@ class TimestampValue {
return TimestampValue(temp);
}
+ /// Return the corresponding timestamp in local time zone for the Unix time specified in
+ /// microseconds.
+ static TimestampValue FromUnixTimeMicros(int64_t unix_time_micros);
+
/// Return the corresponding timestamp in UTC for the Unix time specified in
/// microseconds.
static TimestampValue UtcFromUnixTimeMicros(int64_t unix_time_micros);
@@ -120,22 +124,6 @@ class TimestampValue {
return TimestampValue(temp);
}
- /// Returns the current local time with microsecond accuracy. This should not be used
- /// to time something because it is affected by adjustments to the system clock such
- /// as a daylight savings or a manual correction by a system admin. For timings, use
- /// functions in util/time.h.
- static TimestampValue LocalTime() {
- return TimestampValue(boost::posix_time::microsec_clock::local_time());
- }
-
- /// Returns the current Coordinated Universal Time ("UTC") with microsecond accuracy.
- /// This should not be used to time something because it is affected by adjustments to
- /// the system clock such as a manual correction by a system admin. For timings,
- /// use functions in util/time.h.
- static TimestampValue UtcTime() {
- return TimestampValue(boost::posix_time::microsec_clock::universal_time());
- }
-
/// Returns a TimestampValue converted from a TimestampVal. The caller must ensure
/// the TimestampVal does not represent a NULL.
static TimestampValue FromTimestampVal(const impala_udf::TimestampVal& udf_value) {
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/runtime/timestamp-value.inline.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/timestamp-value.inline.h b/be/src/runtime/timestamp-value.inline.h
index eff78d0..cdabc20 100644
--- a/be/src/runtime/timestamp-value.inline.h
+++ b/be/src/runtime/timestamp-value.inline.h
@@ -36,6 +36,14 @@ inline TimestampValue TimestampValue::UtcFromUnixTimeMicros(int64_t unix_time_mi
return TimestampValue(temp);
}
+inline TimestampValue TimestampValue::FromUnixTimeMicros(int64_t unix_time_micros) {
+ int64_t ts_seconds = unix_time_micros / MICROS_PER_SEC;
+ int64_t micros_part = unix_time_micros - (ts_seconds * MICROS_PER_SEC);
+ boost::posix_time::ptime temp = UnixTimeToLocalPtime(ts_seconds);
+ temp += boost::posix_time::microseconds(micros_part);
+ return TimestampValue(temp);
+}
+
/// Interpret 'this' as a timestamp in UTC and convert to unix time.
/// Returns false if the conversion failed ('unix_time' will be undefined), otherwise
/// true.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/service/client-request-state.cc
----------------------------------------------------------------------
diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index cb542d0..ec4768e 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -88,7 +88,7 @@ ClientRequestState::ClientRequestState(
fetched_rows_(false),
frontend_(frontend),
parent_server_(server),
- start_time_(TimestampValue::LocalTime()) {
+ start_time_us_(UnixMicros()) {
#ifndef NDEBUG
profile_->AddInfoString("DEBUG MODE WARNING", "Query profile created while running a "
"DEBUG build of Impala. Use RELEASE builds to measure query performance.");
@@ -106,7 +106,7 @@ ClientRequestState::ClientRequestState(
summary_profile_->AddInfoString("HiveServer2 Protocol Version",
Substitute("V$0", 1 + session->hs2_version));
}
- summary_profile_->AddInfoString("Start Time", start_time().ToString());
+ summary_profile_->AddInfoString("Start Time", ToStringFromUnixMicros(start_time_us()));
summary_profile_->AddInfoString("End Time", "");
summary_profile_->AddInfoString("Query Type", "N/A");
summary_profile_->AddInfoString("Query State", PrintQueryState(query_state_));
@@ -564,8 +564,8 @@ void ClientRequestState::Done() {
}
unique_lock<mutex> l(lock_);
- end_time_ = TimestampValue::LocalTime();
- summary_profile_->AddInfoString("End Time", end_time().ToString());
+ end_time_us_ = UnixMicros();
+ summary_profile_->AddInfoString("End Time", ToStringFromUnixMicros(end_time_us()));
query_events_->MarkEvent("Unregister query");
// Update result set cache metrics, and update mem limit accounting before tearing
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/service/client-request-state.h
----------------------------------------------------------------------
diff --git a/be/src/service/client-request-state.h b/be/src/service/client-request-state.h
index 1c015c3..0ebdda9 100644
--- a/be/src/service/client-request-state.h
+++ b/be/src/service/client-request-state.h
@@ -186,8 +186,8 @@ class ClientRequestState {
}
const RuntimeProfile* profile() const { return profile_; }
const RuntimeProfile* summary_profile() const { return summary_profile_; }
- const TimestampValue& start_time() const { return start_time_; }
- const TimestampValue& end_time() const { return end_time_; }
+ int64_t start_time_us() const { return start_time_us_; }
+ int64_t end_time_us() const { return end_time_us_; }
const std::string& sql_stmt() const { return query_ctx_.client_request.stmt; }
const TQueryOptions& query_options() const {
return query_ctx_.client_request.query_options;
@@ -338,8 +338,8 @@ class ClientRequestState {
/// catalog update request. Not owned.
ImpalaServer* parent_server_;
- /// Start/end time of the query
- TimestampValue start_time_, end_time_;
+ /// Start/end time of the query, in Unix microseconds.
+ int64_t start_time_us_, end_time_us_;
/// Executes a local catalog operation (an operation that does not need to execute
/// against the catalog service). Includes USE, SHOW, DESCRIBE, and EXPLAIN statements.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/service/impala-http-handler.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-http-handler.cc b/be/src/service/impala-http-handler.cc
index e93aacf..d4ef52f 100644
--- a/be/src/service/impala-http-handler.cc
+++ b/be/src/service/impala-http-handler.cc
@@ -292,21 +292,17 @@ void ImpalaHttpHandler::QueryStateToJson(const ImpalaServer::QueryStateRecord& r
document->GetAllocator());
value->AddMember("stmt_type", stmt_type, document->GetAllocator());
- Value start_time(record.start_time.ToString().c_str(), document->GetAllocator());
+ Value start_time(ToStringFromUnixMicros(record.start_time_us).c_str(),
+ document->GetAllocator());
value->AddMember("start_time", start_time, document->GetAllocator());
- Value end_time(record.end_time.ToString().c_str(), document->GetAllocator());
+ Value end_time(ToStringFromUnixMicros(record.end_time_us).c_str(),
+ document->GetAllocator());
value->AddMember("end_time", end_time, document->GetAllocator());
- const TimestampValue& end_timestamp =
- record.end_time.HasDate() ? record.end_time : TimestampValue::LocalTime();
- double ut_end_time, ut_start_time;
- double duration = 0.0;
- if (LIKELY(end_timestamp.ToSubsecondUnixTime(&ut_end_time))
- && LIKELY(record.start_time.ToSubsecondUnixTime(&ut_start_time))) {
- duration = ut_end_time - ut_start_time;
- }
- const string& printed_duration = PrettyPrinter::Print(duration, TUnit::TIME_S);
+ int64_t duration_us = record.end_time_us - record.start_time_us;
+ const string& printed_duration = PrettyPrinter::Print(duration_us * NANOS_PER_MICRO,
+ TUnit::TIME_NS);
Value val_duration(printed_duration.c_str(), document->GetAllocator());
value->AddMember("duration", val_duration, document->GetAllocator());
@@ -463,19 +459,14 @@ void ImpalaHttpHandler::SessionsHandler(const Webserver::ArgumentMap& args,
Value default_db(state->database.c_str(), document->GetAllocator());
session_json.AddMember("default_database", default_db, document->GetAllocator());
- TimestampValue local_start_time = TimestampValue::FromUnixTime(
- session.second->start_time_ms / 1000);
- local_start_time.UtcToLocal();
- Value start_time(local_start_time.ToString().c_str(), document->GetAllocator());
+ Value start_time(ToStringFromUnixMillis(session.second->start_time_ms,
+ TimePrecision::Second).c_str(), document->GetAllocator());
session_json.AddMember("start_time", start_time, document->GetAllocator());
session_json.AddMember(
"start_time_sort", session.second->start_time_ms, document->GetAllocator());
- TimestampValue local_last_accessed = TimestampValue::FromUnixTime(
- session.second->last_accessed_ms / 1000);
- local_last_accessed.UtcToLocal();
- Value last_accessed(
- local_last_accessed.ToString().c_str(), document->GetAllocator());
+ Value last_accessed(ToStringFromUnixMillis(session.second->last_accessed_ms,
+ TimePrecision::Second).c_str(), document->GetAllocator());
session_json.AddMember("last_accessed", last_accessed, document->GetAllocator());
session_json.AddMember(
"last_accessed_sort", session.second->last_accessed_ms, document->GetAllocator());
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/service/impala-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index 50fe25e..bc8613b 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -470,7 +470,7 @@ Status ImpalaServer::LogAuditRecord(const ClientRequestState& request_state,
writer.String("session_id");
writer.String(PrintId(request_state.session_id()).c_str());
writer.String("start_time");
- writer.String(request_state.start_time().ToString().c_str());
+ writer.String(ToStringFromUnixMicros(request_state.start_time_us()).c_str());
writer.String("authorization_failure");
writer.Bool(Frontend::IsAuthorizationError(request_state.query_status()));
writer.String("status");
@@ -1005,12 +1005,8 @@ Status ImpalaServer::UnregisterQuery(const TUniqueId& query_id, bool check_infli
request_state->Done();
- double ut_end_time, ut_start_time;
- double duration_ms = 0.0;
- if (LIKELY(request_state->end_time().ToSubsecondUnixTime(&ut_end_time))
- && LIKELY(request_state->start_time().ToSubsecondUnixTime(&ut_start_time))) {
- duration_ms = 1000 * (ut_end_time - ut_start_time);
- }
+ int64_t duration_us = request_state->end_time_us() - request_state->start_time_us();
+ int64_t duration_ms = duration_us / MICROS_PER_MILLI;
// duration_ms can be negative when the local timezone changes during query execution.
if (duration_ms >= 0) {
@@ -1665,8 +1661,8 @@ ImpalaServer::QueryStateRecord::QueryStateRecord(const ClientRequestState& reque
stmt_type = request.stmt_type;
effective_user = request_state.effective_user();
default_db = request_state.default_db();
- start_time = request_state.start_time();
- end_time = request_state.end_time();
+ start_time_us = request_state.start_time_us();
+ end_time_us = request_state.end_time_us();
has_coord = false;
Coordinator* coord = request_state.coord();
@@ -1711,8 +1707,8 @@ ImpalaServer::QueryStateRecord::QueryStateRecord(const ClientRequestState& reque
bool ImpalaServer::QueryStateRecordLessThan::operator() (
const QueryStateRecord& lhs, const QueryStateRecord& rhs) const {
- if (lhs.start_time == rhs.start_time) return lhs.id < rhs.id;
- return lhs.start_time < rhs.start_time;
+ if (lhs.start_time_us == rhs.start_time_us) return lhs.id < rhs.id;
+ return lhs.start_time_us < rhs.start_time_us;
}
void ImpalaServer::ConnectionStart(
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/service/impala-server.h
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.h b/be/src/service/impala-server.h
index dbd2877..1653765 100644
--- a/be/src/service/impala-server.h
+++ b/be/src/service/impala-server.h
@@ -619,8 +619,8 @@ class ImpalaServer : public ImpalaServiceIf,
/// The state of the query as of this snapshot
beeswax::QueryState::type query_state;
- /// Start and end time of the query
- TimestampValue start_time, end_time;
+ /// Start and end time of the query, in Unix microseconds.
+ int64_t start_time_us, end_time_us;
/// Summary of execution for this query.
TExecSummary exec_summary;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/statestore/statestore-subscriber.cc
----------------------------------------------------------------------
diff --git a/be/src/statestore/statestore-subscriber.cc b/be/src/statestore/statestore-subscriber.cc
index fa839aa..4c5d480 100644
--- a/be/src/statestore/statestore-subscriber.cc
+++ b/be/src/statestore/statestore-subscriber.cc
@@ -275,7 +275,7 @@ void StatestoreSubscriber::RecoveryModeChecker() {
// we would otherwise have to cache updates here.
last_recovery_duration_metric_->set_value(
recovery_timer.ElapsedTime() / (1000.0 * 1000.0 * 1000.0));
- last_recovery_time_metric_->set_value(TimestampValue::LocalTime().ToString());
+ last_recovery_time_metric_->set_value(CurrentTimeString());
}
SleepForMs(SLEEP_INTERVAL_MS);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/util/common-metrics.cc
----------------------------------------------------------------------
diff --git a/be/src/util/common-metrics.cc b/be/src/util/common-metrics.cc
index 94c32b5..d147862 100644
--- a/be/src/util/common-metrics.cc
+++ b/be/src/util/common-metrics.cc
@@ -16,7 +16,7 @@
// under the License.
#include "util/common-metrics.h"
-#include "runtime/timestamp-value.h"
+#include "util/time.h"
#include <kudu/client/client.h>
namespace impala {
@@ -33,8 +33,7 @@ void CommonMetrics::InitCommonMetrics(MetricGroup* metric_group) {
KUDU_CLIENT_VERSION = metric_group->AddProperty<string>(
KUDU_CLIENT_VERSION_METRIC_NAME, kudu::client::GetShortVersionString());
- // TODO: IMPALA-5599: Clean up non-TIMESTAMP usages of TimestampValue
- PROCESS_START_TIME->set_value(TimestampValue::LocalTime().ToString());
+ PROCESS_START_TIME->set_value(CurrentTimeString());
}
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1640aa97/be/src/util/time.h
----------------------------------------------------------------------
diff --git a/be/src/util/time.h b/be/src/util/time.h
index 5e9a4fd..d636183 100644
--- a/be/src/util/time.h
+++ b/be/src/util/time.h
@@ -103,5 +103,10 @@ std::string ToStringFromUnixMicros(int64_t us,
std::string ToUtcStringFromUnixMicros(int64_t us,
TimePrecision p = TimePrecision::Microsecond);
+/// Convenience function to convert the current time, derived from UnixMicros(),
+/// to a date-time string in the local time zone.
+inline std::string CurrentTimeString() {
+ return ToStringFromUnixMicros(UnixMicros());
+}
} // namespace impala
#endif