You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/10/15 19:32:54 UTC
[kudu-CR] [clock] handle corner case of NowWithError() in Update()
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14456
Change subject: [clock] handle corner case of NowWithError() in Update()
......................................................................
[clock] handle corner case of NowWithError() in Update()
This patch addresses the corner case of HybridClock::NowWithError()
in HybridClock::Update(), accounting for the higher error threshold
than specified with the --max_clock_sync_error_usec flag. The corner
case is about the situation when physical timestamp candidate derived
from current readings of the wallclock is not ahead of the next
scheduled timestamp. In that case, the estimated error returned
from HybridClock::NowWithError() might be higher than returned by
HybridClock::WalltimeWithError().
Change-Id: I701eed492442bc6360d4c8f02f1bfc3ee96a3b90
---
M src/kudu/clock/hybrid_clock.cc
1 file changed, 11 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/14456/1
--
To view, visit http://gerrit.cloudera.org:8080/14456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I701eed492442bc6360d4c8f02f1bfc3ee96a3b90
Gerrit-Change-Number: 14456
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [clock] handle corner case of NowWithError() in Update()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14456 )
Change subject: [clock] handle corner case of NowWithError() in Update()
......................................................................
Patch Set 1:
(2 comments)
Could we test this, perhaps using the mock time source?
http://gerrit.cloudera.org:8080/#/c/14456/1/src/kudu/clock/hybrid_clock.cc
File src/kudu/clock/hybrid_clock.cc:
http://gerrit.cloudera.org:8080/#/c/14456/1/src/kudu/clock/hybrid_clock.cc@218
PS1, Line 218: void HybridClock::NowWithError(Timestamp* timestamp, uint64_t* max_error_usec) {
On a related note, I'm pretty sure NowWithError must be called with lock_ held. So we should rename it to Unlocked, add an AssertLocked, and probably make it private too.
http://gerrit.cloudera.org:8080/#/c/14456/1/src/kudu/clock/hybrid_clock.cc@258
PS1, Line 258: DCHECK_GE(next_timestamp_ >> kBitsToShift, now_usec - error_usec);
Maybe pull these out of the DCHECK and into locals so they can be shared with L259?
--
To view, visit http://gerrit.cloudera.org:8080/14456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I701eed492442bc6360d4c8f02f1bfc3ee96a3b90
Gerrit-Change-Number: 14456
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 15 Oct 2019 20:22:27 +0000
Gerrit-HasComments: Yes