[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-05 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1884/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 05 Feb 2018 18:46:57 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-05 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG@13
PS1, Line 13: by switching to
> Makes sense. Let's just add that to the commit message then, so it's useful
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 05 Feb 2018 18:45:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-05 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 2: Code-Review+2

Carry +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 05 Feb 2018 18:46:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-05 Thread Michael Ho (Code Review) 
Hello Jim Apple, Sailesh Mukil,

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

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

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

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..

IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable

ConditionVariable is a thin wrapper around pthread_cond_*.
Currently, pthread_cond_timedwait() uses the default attribute
CLOCK_REALTIME. This is susceptible to adjustment to the system
clock from various sources such as NTP and time may go backward.
This change fixes the problem by switching to using CLOCK_MONOTONIC
so time will be monotonic although the frequency of the clock ticks
may still be adjusted by NTP. Ideally, we should use CLOCK_MONOTONIC_RAW
but it's available only on Linux kernel 2.6.28 or latter. This change
also get rids of some usage of boost::get_system_time() which suffers
from the same problem.

Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
---
M be/src/rpc/thrift-server.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/service/impala-server.cc
M be/src/util/blocking-queue.h
M be/src/util/condition-variable.h
M be/src/util/promise.h
M be/src/util/time.h
7 files changed, 44 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/9158/2
--
To view, visit http://gerrit.cloudera.org:8080/9158
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-02 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG@13
PS1, Line 13: CLOCK_MONOTONIC
> Thanks for doing the research. I believe CLOCK_MONOTONIC_RAW is only availa
Makes sense. Let's just add that to the commit message then, so it's useful for 
future reference.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:36:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-02 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG@13
PS1, Line 13: CLOCK_MONOTONIC
> Reading the man pages, it basically says that even CLOCK_MONOTONIC can be a
Thanks for doing the research. I believe CLOCK_MONOTONIC_RAW is only available 
after Linux kernel 2.6.28 so CLOCK_MONOTONIC seems more widely available.

https://www.systutorials.com/docs/linux/man/2-clock_settime/

FWIW, our monotonic stopwatch is also using CLOCK_MONOTONIC. The monotonicity 
of the clock is the major point here. I agree that CLOCK_MONOTONIC_RAW would be 
even better but we may need to do more work to detect whether it's available on 
the platform.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:09:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-02 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9158/1//COMMIT_MSG@13
PS1, Line 13: CLOCK_MONOTONIC
Reading the man pages, it basically says that even CLOCK_MONOTONIC can be 
affected by sources like NTP. However, the difference is that with 
CLOCK_MONOTONIC, the time will not jump backwards, but rather experience "time 
slew", which means that the frequency of clock ticks can change momentarily to 
reflect adjustments in NTP.

CLOCK_MONOTONIC_RAW on the other hand shows the absolute wall clock time that's 
not affected by NTP. But it could be affected by things like temperature, 
environment, etc. that will not be corrected (CLOCK_MONOTONIC would have that 
corrected apparently)

It seems a bit confusing which would be considered "right" while measuring time 
like we do. I'm okay with both, as long as we've considered both the options.

Whatever you think is the right option, I think we can just add to the commit 
message that we've considered the other one and give a reason as to why we 
didn't choose that.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:03:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-02-02 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9158/1/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:

http://gerrit.cloudera.org:8080/#/c/9158/1/be/src/util/condition-variable.h@39
PS1, Line 39: pthread_condattr_setclock(, CLOCK_MONOTONIC);
> CLOCK_MONOTONIC_COARSE may be a bit too coarse for our purpose here given s
WFM



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 02 Feb 2018 16:40:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-01-30 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9158 )

Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9158/1/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:

http://gerrit.cloudera.org:8080/#/c/9158/1/be/src/util/condition-variable.h@39
PS1, Line 39: pthread_condattr_setclock(, CLOCK_MONOTONIC);
> CLOCK_MONOTONIC can be very slow on EC2: IMPALA-2407
CLOCK_MONOTONIC_COARSE may be a bit too coarse for our purpose here given some 
callers of ConditionVariable do pass in micro-seconds duration.

My understanding from internet sources 
(https://blog.packagecloud.io/eng/2017/03/08/system-calls-are-much-slower-on-ec2/)
 about this slowness is that gettimeofday() (which boost::get_system_time() 
will call) is in the same ballpark as clock_gettime(CLOCK_MONOTONIC, ...) so 
it's not exactly regressing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 31 Jan 2018 00:18:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6449: Use CLOCK MONOTONIC in ConditionVariable

2018-01-29 Thread Michael Ho (Code Review) 
Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9158


Change subject: IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable
..

IMPALA-6449: Use CLOCK_MONOTONIC in ConditionVariable

ConditionVariable is a thin wrapper around pthread_cond_*.
Currently, pthread_cond_timedwait() uses the default attribute
CLOCK_REALTIME. This is susceptible to adjustment to the system
clock from various sources such as NTP. This change fixes the
problem by switching to using CLOCK_MONOTONIC instead. This change
also get rids of some usage of boost::get_system_time() which
suffers from the same problem.

Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
---
M be/src/rpc/thrift-server.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/service/impala-server.cc
M be/src/util/blocking-queue.h
M be/src/util/condition-variable.h
M be/src/util/promise.h
M be/src/util/time.h
7 files changed, 44 insertions(+), 36 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/9158/1
--
To view, visit http://gerrit.cloudera.org:8080/9158
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I81611cfd5e7c5347203fe7fa6b0f615602257f87
Gerrit-Change-Number: 9158
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho