You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/12/02 12:24:09 UTC

[kudu-CR] Allow to build a MonoTime from a MonoDelta 

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................

Allow to build a MonoTime from a MonoDelta

This allows to build a MonoTime from a timestamp representing
time since the "start". This is purposefully vague as the source
of time can the machine's monotonic clock or something else
like time since the epoch.

Because of this care should be taken to only compare MonoTime
instances from the same source.

In order to perform the transformation, first the caller transform
the timestamp to a MonoDelta to take advantage of the unit
handling methods. Then MonoTime::FromDeltaSinceTheEpoch()
can be called to get a MonoTime representing that timestamp.

Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
---
M src/kudu/util/monotime-test.cc
M src/kudu/util/monotime.cc
M src/kudu/util/monotime.h
3 files changed, 26 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#3).

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................

Allow to build a MonoTime from a MonoDelta

This allows to build a MonoTime from a timestamp representing
time since the "start". This is purposefully vague as the source
of time can the machine's monotonic clock or something else
like time since the epoch.

Because of this care should be taken to only compare MonoTime
instances from the same source.

In order to perform the transformation, first the caller transform
the timestamp to a MonoDelta to take advantage of the unit
handling methods. Then MonoTime::FromDeltaSinceTheEpoch()
can be called to get a MonoTime representing that timestamp.

Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
---
M src/kudu/util/monotime-test.cc
M src/kudu/util/monotime.cc
M src/kudu/util/monotime.h
3 files changed, 63 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5325/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................


Patch Set 2:

the point is to be able to get a monotime from the hybrid clock like in here: https://gerrit.cloudera.org/#/c/5305/3/src/kudu/consensus/time_manager.cc

I like the idea of having an enum for the domain. will do

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................


Patch Set 2:

> the point is to be able to get a monotime from the hybrid clock
 > like in here: https://gerrit.cloudera.org/#/c/5305/3/src/kudu/consensus/time_manager.cc
 > 
 > I like the idea of having an enum for the domain. will do


Thank you for the reference to the sample code.  After looking into that code, I added a comment on the in-line, but just in case I'll re-iterate it right here:


Instead of the line of conversions like

(Timestamp, Timestamp) --> (MonoTime, MonoTime) --> MonoDelta

and creating MonoTime objects in different domains, why not to implement operator-(const Timestamp&, const Timestamp&) for getting difference of timestamps and then just do

MonoDelta diff(MonoDelta::FromMicroseconds(clock.GetPhysicalValueMicros(t1 - t0)))

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................


Patch Set 1:

so that we can build a monotime from a timestamp (like the physical part of a timestamp) and use it for calculations and to add deltas and whatnot.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................


Patch Set 1:

Why? This is kind of weird

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has abandoned this change.

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................


Abandoned

abandoning this for a better solution

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has uploaded a new patch set (#2).

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................

Allow to build a MonoTime from a MonoDelta

This allows to build a MonoTime from a timestamp representing
time since the "start". This is purposefully vague as the source
of time can the machine's monotonic clock or something else
like time since the epoch.

Because of this care should be taken to only compare MonoTime
instances from the same source.

In order to perform the transformation, first the caller transform
the timestamp to a MonoDelta to take advantage of the unit
handling methods. Then MonoTime::FromDeltaSinceStart()
can be called to get a MonoTime representing that timestamp.

Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
---
M src/kudu/util/monotime-test.cc
M src/kudu/util/monotime.cc
M src/kudu/util/monotime.h
3 files changed, 26 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5325/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#4).

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................

Allow to build a MonoTime from a MonoDelta

This allows to build a MonoTime from a timestamp representing
time since the "start". This is purposefully vague as the source
of time can the machine's monotonic clock or something else
like time since the epoch.

Because of this care should be taken to only compare MonoTime
instances from the same source.

In order to perform the transformation, first the caller transform
the timestamp to a MonoDelta to take advantage of the unit
handling methods. Then MonoTime::FromDeltaSinceTheEpoch()
can be called to get a MonoTime representing that timestamp.

Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
---
M src/kudu/util/monotime-test.cc
M src/kudu/util/monotime.cc
M src/kudu/util/monotime.h
3 files changed, 60 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5325/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Allow to build a MonoTime from a MonoDelta

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Allow to build a MonoTime from a MonoDelta
......................................................................


Patch Set 2:

> so that we can build a monotime from a timestamp (like the physical
 > part of a timestamp) and use it for calculations and to add deltas
 > and whatnot.

This sounds a little bit questionable to me.  What is the target space of the MonoTime objects built such a way?  If it's just Timestamp of uint64_t, I don't think this is needed.

Also, what do you think of adding some guardrails to prevent unintentional comparison of MonoTime out from different domains?  Like, add some sort of parameter for MonoTime constructor which would represent the domain/field of the object.  The comparison operations might them check the domain in debug builds.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I100e72cf4ac5cf23570e69e32a998253a63c6505
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No