[kudu-CR] Refactor HybridClock and add a local clock mode

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

I ended up pulling some of this idea into 
de8b92eb6469f843d43c432678e52732a861014b

though it seems this patch does a bit more than the simple refactoring there. 
Additionally in the above patch I only enabled the "system unsynchronized time" 
on OSX

-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Refactor HybridClock and add a local clock mode

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 45: DEFINE_bool(use_hybrid_clock, true,
> just for internal tests and hidden + unsafe, yeah.
how about --clock=system_ntp, --clock=local, --clock=logical_only? Even for 
internal-only usage, it's a little weird to have one flag which essentially 
disables the usage of the other flag.


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 45: DEFINE_bool(use_hybrid_clock, true,
> just for internal tests and hidden + unsafe, yeah.
what about --clock={logical,local,system_ntp,kntp} (assuming you call your 
builtin ntp 'kntp')


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

todd: just for internal tests and hidden + unsafe, yeah.
mj: my problem with todd's -wall_clock=none is that it doesn't make clear that 
we're using the logical clock (kind of like today's --use_hybrid_clock=false 
doesn't either).
Also none of these are "user" flags, all are hidden and just something we use 
locally. In real clusters --wall_clock=local might cause issues we haven't 
considered and should be just a work aroind

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 45: DEFINE_bool(use_hybrid_clock, true,
> hm, what's the point of the logical clock? it's just for our own internal u
just for internal tests and hidden + unsafe, yeah.


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

If it's possible to avoid the extra flag, that would be awesome, it makes it 
harder to understand. If I'm getting confused I suspect some users will as well.

-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Refactor HybridClock and add a local clock mode

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 45: DEFINE_bool(use_hybrid_clock, true,
> my bad. I meant to remove/deprecate the the -use_hybrid_clock one. My sugge
hm, what's the point of the logical clock? it's just for our own internal unit 
tests where we want complete determinism of timestamps? (ie it can be 
hidden+unsafe?)


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 45: DEFINE_bool(use_hybrid_clock, true,
> getting a little confused about the 2x2 matrix of valid options now.
my bad. I meant to remove/deprecate the the -use_hybrid_clock one. My 
suggestion is that we have:
--use_logical_clock (default false)
If the flag above is false then we'd have:
--wall_clock=mock
--wall_clock=system_ntp
--wall_clock=local
(and eventually --wall_clock=builtin_ntp)

the deprecated flag would be the negation of --use_logical_clock


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/common/timestamp.cc
File src/kudu/common/timestamp.cc:

PS1, Line 55:   
> nit extra space
strikes me as a little odd that we are just assuming kInvalidTimestamp here is 
the max value. Maybe worth a second constant like 'kMaxValidTimestamp'?

Also strikes me as strange that we just silently don't increment in the case 
that we've reached the max valid one.

Wouldn't this also allow incrementing from max valid to kInvalid?


http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 45: DEFINE_bool(use_hybrid_clock, true,
getting a little confused about the 2x2 matrix of valid options now.

Should we deprecate 'use_hybrid_clock' and _always_ use the hybrid clock, but 
with a new flag like:

--wall_clock=system_ntp
[would be like our current defaults]

--wall_clock=local
[would disable COMMIT_WAIT and potentially have some weird results if you use 
wall-clock based snapshots]

--wall_clock=none
[would be completely logical-clock based? is this even useful?]

then we have an easy extension point if we decide to implement our own NTP:

--wall_clock=builtin_ntp
[use our own SNTP implementation of some sort and ignore the system clock]


http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.h
File src/kudu/server/hybrid_clock.h:

PS1, Line 164:   struct PhysicalClockStatus {
 : // The clock API that should be used.
 : enum ClockMode {
 :   // Whether the NTP api should be used.
 :   NTP,
 :   // Whether the "local" api should be used. In macOS this 
is the only mode available.
 :   LOCAL,
 :   // Whether to use a "mock" clock
 :   MOCK
 : };
 : ClockMode clock_mode;
 : double tolerance_adjustment;
 : uint64_t divisor;
 : // Set when the mock clock is enabled. For testing purposes 
only.
 : uint64_t mock_clock_time_usec_;
 : uint64_t mock_clock_max_error_usec_;
 : 
 : uint64_t to_microseconds(uint64_t clock_read) {
 :   return clock_read / divisor;
 : }
 :   };
> A status seems like something that represents a particular state in time, i
+1. Would it make sense at this point to actually just create a new interface 
like 'PhysicalClock' or 'WallClock', and have the HybridClock contain a 
reference to the PhysicalClock or WallClock? Then the physical clock can easily 
expose the various getters needed above (and will be nice and easy to sub in 
new implementations, either for testing (as in mock clock) or if we decide to 
implement a custom NTP, etc)


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

PS1, Line 45: DEFINE_bool(use_hybrid_clock, true,
: "Whether HybridClock should be used as the default 
clock"
: " implementation. This should be disabled for testing 
purposes only.");
: TAG_FLAG(use_hybrid_clock, hidden);
: 
: DEFINE_bool(use_unsynchronized_clock, false,
Thinking about this more, why don't we just always use the local clock when the 
hybrid clock is disabled? We know that the combination of use_hybrid = true 
with use_unsynchronized = true will not work.


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(3 comments)

thanks!

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/common/timestamp.cc
File src/kudu/common/timestamp.cc:

PS1, Line 55:   
nit extra space


http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.h
File src/kudu/server/hybrid_clock.h:

PS1, Line 164:   struct PhysicalClockStatus {
 : // The clock API that should be used.
 : enum ClockMode {
 :   // Whether the NTP api should be used.
 :   NTP,
 :   // Whether the "local" api should be used. In macOS this 
is the only mode available.
 :   LOCAL,
 :   // Whether to use a "mock" clock
 :   MOCK
 : };
 : ClockMode clock_mode;
 : double tolerance_adjustment;
 : uint64_t divisor;
 : // Set when the mock clock is enabled. For testing purposes 
only.
 : uint64_t mock_clock_time_usec_;
 : uint64_t mock_clock_max_error_usec_;
 : 
 : uint64_t to_microseconds(uint64_t clock_read) {
 :   return clock_read / divisor;
 : }
 :   };
A status seems like something that represents a particular state in time, i.e. 
something that changes, so I think this abstraction is a bit confusing. It 
seems more like a definition of the clock's properties. How about 
PhysicalClockDef or PhysicalClockProperties ?


PS1, Line 186:   Status InitPhysicalClock();
comment that the physical clock properties or whatever you decide to call it 
above should not change after this method, except for testing. Ideally it'd be 
nice to have the non-changing parts made const to make it more clear.


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: Refactor HybridClock and add a local clock mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5982/1/src/kudu/server/hybrid_clock.cc
File src/kudu/server/hybrid_clock.cc:

Line 53: "This should be disabled for testing purposes only.");
I think you meant "This should be enabled*"


-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Refactor HybridClock and add a local clock mode

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5982

Change subject: Refactor HybridClock and add a local clock mode
..

Refactor HybridClock and add a local clock mode

Some test clusters have really unstable NTP meaning that it will
report as unsynchronized often, making tests fail. To address
this, this patch adds a new clock mode that just reads from the local
clock. As long as there are no COMMIT_WAIT operations this should
still be correct. However since we're now going to use the hybrid
clock in a distributed setting we need to make extra sure that
we're not making assumptions based on the clock error. In order
to do that the hybrid clock should crash, in local clock mode, if
the error is ever requested. The previous layout of the HybridClock
class wasn't very amenable to isolating the calls that just need
the time vs the ones that need both the time and the error. This
patch refactors things so that they are isolated.

This patch does the following changes:
- It adds a new "local" clock mode that can be used to obtain
time, but not the clock error.
- It makes it so it's possible to get the current time without
increasing the clock value, which allowed to coalesce some
code.
- It merges the ifdefs into a single block and makes changes
to the other methods accordingly.
- It makes it so that the clock crashes when an error is requested
(directly or indirectly) instead of returning an error of 0.
This required making some changes to a few places, mostly tests,
but since the new clock mode will have no error accounting and its
likely to be much more broadly used than the mock clock this makes
sure that there aren't any operations being performed that rely
on the error for correctness. The single exception is that we
still report an error of 0 for metrics.
- Adds operator++ to Timestamp so that we can avoid converting
to/from uint64_t all the time.

Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock-test.cc
M src/kudu/server/hybrid_clock.cc
M src/kudu/server/hybrid_clock.h
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 336 insertions(+), 229 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/5982/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I78d616f64558f4b9a95535ebc8702f75a37dad39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves