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 2018/05/10 21:02:49 UTC
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
David Ribeiro Alves has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10371
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Fix int overflow GetClockTimeMicros() on macOS
On macOS mach_timespec_t.tv_sec is only 4 bytes and we
were converting to micros before moving to a bigger var.
This would cause all the wall times obtained on a mac (through
GetClockTimeMicros() to be wrong.
This was likely the cause of KUDU-2435 and KUDU-2408 too, since
the time would easily wrap, causing us to a update the clock with
a value that was seemingly from the future.
Posix just requires it to be an integer
(see: https://en.wikipedia.org/w/index.php?title=Time_t&oldid=450752800)
so also fixed it on the non-macOS path.
Testing this is likely not worth it since the only real change
was on macOS where the overlow doesn't happen anymore.
Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
---
M src/kudu/gutil/walltime.h
1 file changed, 11 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/10371/1
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 10 May 2018 22:03:21 +0000
Gerrit-HasComments: No
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h
File src/kudu/gutil/walltime.h:
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h@87
PS1, Line 87: micros_from_secs *= 1000 * 1000 + ts.tv_nsec / 1000;
Is the precedence here correct? Seems like it's now
MicrosecondsInt64 micros_from_secs = static_cast<MicrosecondsInt64>(ts.tv_sec) * ((1000 * 1000) + (ts.tvnsec / 1000))
which isn't right. Regardless of whether it's right, the *= with + is confusing, so maybe split it out.
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h@141
PS1, Line 141: micros_from_secs *= 1000 * 1000 + ts.tv_nsec / 1000;
same
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 10 May 2018 21:09:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10371
to look at the new patch set (#2).
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Fix int overflow GetClockTimeMicros() on macOS
On macOS mach_timespec_t.tv_sec is only 4 bytes and we
were converting to micros before moving to a bigger var.
This would cause all the wall times obtained on a mac (through
GetClockTimeMicros() to be wrong.
This was likely the cause of KUDU-2435 and KUDU-2408 too, since
the time would easily wrap, causing us to a update the clock with
a value that was seemingly from the future.
Posix just requires it to be an integer
(see: https://en.wikipedia.org/w/index.php?title=Time_t&oldid=450752800)
so also fixed it on the non-macOS path.
Testing this is likely not worth it since the only real change
was on macOS where the overlow doesn't happen anymore.
Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
---
M src/kudu/gutil/walltime.h
1 file changed, 13 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/10371/2
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h
File src/kudu/gutil/walltime.h:
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h@87
PS1, Line 87: micros_from_secs *= 1000 * 1000 + ts.tv_nsec / 1000;
> Is the precedence here correct? Seems like it's now
Done
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h@141
PS1, Line 141: micros_from_secs *= 1000 * 1000 + ts.tv_nsec / 1000;
> same
Done
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 10 May 2018 21:49:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 10 May 2018 21:54:55 +0000
Gerrit-HasComments: No
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Patch Set 2: Code-Review+2
This passes my failing Java tests on mac.
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 10 May 2018 22:04:20 +0000
Gerrit-HasComments: No
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h
File src/kudu/gutil/walltime.h:
http://gerrit.cloudera.org:8080/#/c/10371/1/src/kudu/gutil/walltime.h@87
PS1, Line 87: micros_from_secs *= 1000 * 1000 + ts.tv_nsec / 1000;
> Is the precedence here correct? Seems like it's now
ah, good point, changed it just before I pushed to have one less line.
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 10 May 2018 21:46:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix int overflow GetClockTimeMicros() on macOS
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10371 )
Change subject: Fix int overflow GetClockTimeMicros() on macOS
......................................................................
Fix int overflow GetClockTimeMicros() on macOS
On macOS mach_timespec_t.tv_sec is only 4 bytes and we
were converting to micros before moving to a bigger var.
This would cause all the wall times obtained on a mac (through
GetClockTimeMicros() to be wrong.
This was likely the cause of KUDU-2435 and KUDU-2408 too, since
the time would easily wrap, causing us to a update the clock with
a value that was seemingly from the future.
Posix just requires it to be an integer
(see: https://en.wikipedia.org/w/index.php?title=Time_t&oldid=450752800)
so also fixed it on the non-macOS path.
Testing this is likely not worth it since the only real change
was on macOS where the overlow doesn't happen anymore.
Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Reviewed-on: http://gerrit.cloudera.org:8080/10371
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Grant Henke <gr...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/gutil/walltime.h
1 file changed, 13 insertions(+), 2 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, but someone else must approve
Dan Burkert: Looks good to me, approved
Grant Henke: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie0eaa548f61352be529755a732566613cfa72098
Gerrit-Change-Number: 10371
Gerrit-PatchSet: 3
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>