HIVE-26670: Track every single HTTP request between beeline and hs2 #3710
Conversation
There was a problem hiding this comment.
Thanks, @abstractdog ! This looks useful. I entered a few comments. Additionally, have you explored if any unit testing is possible?
jdbc/src/java/org/apache/hive/jdbc/HttpRequestInterceptorBase.java
Outdated
Show resolved
Hide resolved
jdbc/src/java/org/apache/hive/jdbc/HttpResponseInterceptorBase.java
Outdated
Show resolved
Hide resolved
jdbc/src/java/org/apache/hive/jdbc/HttpResponseInterceptorBase.java
Outdated
Show resolved
Hide resolved
thanks a lot @cnauroth! honestly, I haven't found existing unit tests in this area to extend easily, and I was lazy :) but it's time now to improve, let me check |
abstractdog
force-pushed
the
HIVE-26670
branch
from
dd28dea
to
7579d5f
Compare
There was a problem hiding this comment.
@abstractdog , thanks for incorporating the code review feedback! I have just a few more questions.
jdbc/src/java/org/apache/hive/jdbc/HttpRequestInterceptorBase.java
Outdated
Show resolved
Hide resolved
abstractdog
force-pushed
the
HIVE-26670
branch
from
7579d5f
to
a77028b
Compare
There was a problem hiding this comment.
+1 (non-binding)
@abstractdog , thanks for your patience responding to the code review feedback!
abstractdog
force-pushed
the
HIVE-26670
branch
from
a77028b
to
9d94127
Compare
thanks a lot @cnauroth for the quality code review! |
kgyrtkirk
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
| if (trackHeader == null) { | ||
| return; | ||
| } | ||
| long elapsed = System.currentTimeMillis() - (long) context.getAttribute(trackHeader + "_TIME"); |
There was a problem hiding this comment.
nit:
Can use Constants.TIME_POSTFIX_REQUEST_TRACK instead of hardcoding _TIME here
There was a problem hiding this comment.
right, night catch! that's why I created this constant
fixing it
| @@ -813,6 +817,27 @@ RegistryBuilder.<ConnectionSocketFactory> create().register("https", socketFacto | |||
| return httpClientBuilder.build(); | |||
| } | |||
|
|
|||
| private boolean isRequestTrackingEnabled() { | |||
| return Boolean.valueOf(sessConfMap.get(JdbcConnectionParams.JDBC_PARAM_REQUEST_TRACK)); | |||
There was a problem hiding this comment.
Can replace with:
return Boolean.parseBoolean(sessConfMap.get(JdbcConnectionParams.JDBC_PARAM_REQUEST_TRACK));
Boolean.valueOf does a parseBoolean which returns a primitive boolean, which is converted into Boolean and now you are again returning a primitive boolean, the conversion from boolean to Boolean is unnecessary
| LOG.info("{}:{}", Constants.HTTP_HEADER_REQUEST_TRACK, trackHeader); | ||
| additionalHeaders.put(Constants.HTTP_HEADER_REQUEST_TRACK, trackHeader); | ||
| httpContext.setAttribute(Constants.HTTP_HEADER_REQUEST_TRACK, trackHeader); | ||
| httpContext.setAttribute(trackHeader + Constants.TIME_POSTFIX_REQUEST_TRACK, System.currentTimeMillis()); |
There was a problem hiding this comment.
We are using System.currentTimeMillis() to calculate the duration. I am not very sure, if using Time.monotonicNow() Will be better.
Just by the doc:
https://github.com/apache/hadoop/blob/b1f418f8027c2d2a1a1c17ec3a8d23ceaaf8d89c/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Time.java#L49-L69
There was a problem hiding this comment.
yeah, I tend to use System.currentTimeMillis(), hadoop util looks better clearly according to javadoc
abstractdog
force-pushed
the
HIVE-26670
branch
from
9d94127
to
ae3ef10
Compare
thanks, fixed them! |
|
Kudos, SonarCloud Quality Gate passed!
|
…pache#3710) (Laszlo Bodor reviewed by Ayush Saxena, Chris Nauroth)
…pache#3710) (Laszlo Bodor reviewed by Ayush Saxena, Chris Nauroth) (cherry picked from commit 1afd305)
…pache#3710) (Laszlo Bodor reviewed by Ayush Saxena, Chris Nauroth)
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?