[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

2017-10-18 Thread Zoram Thanga (Code Review) 
Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h@341
PS1, Line 341: time
> indicate the clock. e.g. monotonic vs unix time.
Done. This uses Unix time.


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.cc@a91
PS1, Line 91:
> What do you think about going a step further and replacing all callers of T
Changed the caller in be/src/statetore. The other calls in be/src/exprs are in 
test code, and seem legit


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
> That's true but that also seems to be a pre-existing bug. I agree that we s
Please see my response to DanH's comment...


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
> that can be negative if settimeofday() occurred in the mean time. What shou
The existing code does not handle this either. Looking at 
PrettyPrinter::Print(), it will basically print a negative elapsed time in 
nanoseconds. The only way to reliably handle intervening settimeofday(), or 
something like that would be to use a monotonic clock, perhaps 
MonotonicStopWatch.

Are you suggesting we go down that route?


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@622
PS1, Line 622: /// Start and end time of the query
> same comment about clock
Done


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@640
PS1, Line 640: Unix milliseconds
> like that
Done


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.cc@1009
PS1, Line 1009:   int64_t duration_ms = duration_us / MICROS_PER_MILLI;
> same question (though old code could go negative).
Please see my earlier response.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 18 Oct 2017 22:24:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

2017-10-18 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.cc@a91
PS1, Line 91:
What do you think about going a step further and replacing all callers of 
TimestampValue::LocalTime() with UnixMicros() or the like ? There doesn't seem 
to be many callers left after this patch.


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
> that can be negative if settimeofday() occurred in the mean time. What shou
That's true but that also seems to be a pre-existing bug. I agree that we 
should handle it though.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 18 Oct 2017 19:22:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

2017-10-17 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h@341
PS1, Line 341: time
indicate the clock. e.g. monotonic vs unix time.


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
that can be negative if settimeofday() occurred in the mean time. What should 
happen in that case?


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@622
PS1, Line 622: /// Start and end time of the query
same comment about clock


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@640
PS1, Line 640: Unix milliseconds
like that


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.cc@1009
PS1, Line 1009:   int64_t duration_ms = duration_us / MICROS_PER_MILLI;
same question (though old code could go negative).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:42:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

2017-10-17 Thread Zoram Thanga (Code Review) 
Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8305


Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..

IMPALA-5599: Clean up references to TimestampValue in be/src/service.

This is a follow-on commit to d53f43b4. In this patch we replace all
inappropriate usage of TimestampValue in be/src/service with simpler
data types for Unix timestamps, and where conversions to strings are
needed, we use the interfaces introduced by the previous commit.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 26 insertions(+), 39 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga