You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/02/12 19:35:21 UTC
[GitHub] [hive] belugabehr opened a new pull request #1974: HIVE-24772: Revamp Server Request Error Logging
belugabehr opened a new pull request #1974:
URL: https://github.com/apache/hive/pull/1974
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr closed pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr closed pull request #1974:
URL: https://github.com/apache/hive/pull/1974
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-824393358
@zchovan @aihuaxu Can you please take another look? Got the tests passing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr closed pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr closed pull request #1974:
URL: https://github.com/apache/hive/pull/1974
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1974:
URL: https://github.com/apache/hive/pull/1974#discussion_r581162667
##########
File path: service/src/java/org/apache/hive/service/cli/HiveSQLException.java
##########
@@ -102,149 +115,39 @@ public HiveSQLException(String reason, String sqlState, int vendorCode, Throwabl
}
public HiveSQLException(TStatus status) {
- // TODO: set correct vendorCode field
super(status.getErrorMessage(), status.getSqlState(), status.getErrorCode());
- if (status.getInfoMessages() != null) {
- initCause(toCause(status.getInfoMessages()));
- }
}
-
-
-/**
- * Converts current object to a {@link TStatus} object
- * @return a {@link TStatus} object
+ /**
+ * Converts current object to a {@link TStatus} object.
+ *
+ * @return a {@link TStatus} object
*/
public TStatus toTStatus() {
// TODO: convert sqlState, etc.
TStatus tStatus = new TStatus(TStatusCode.ERROR_STATUS);
tStatus.setSqlState(getSQLState());
tStatus.setErrorCode(getErrorCode());
tStatus.setErrorMessage(getMessage());
- tStatus.setInfoMessages(toString(this));
+ tStatus.setInfoMessages(DEFAULT_INFO);
return tStatus;
}
/**
- * Converts the specified {@link Exception} object into a {@link TStatus} object
- * @param e a {@link Exception} object
- * @return a {@link TStatus} object
+ * Converts the specified {@link Exception} object into a {@link TStatus}
+ * object.
+ *
+ * @param e a {@link Exception} object
+ * @return a {@link TStatus} object
*/
public static TStatus toTStatus(Exception e) {
if (e instanceof HiveSQLException) {
- return ((HiveSQLException)e).toTStatus();
+ return ((HiveSQLException) e).toTStatus();
}
TStatus tStatus = new TStatus(TStatusCode.ERROR_STATUS);
tStatus.setErrorMessage(e.getMessage());
- tStatus.setInfoMessages(toString(e));
+ tStatus.setInfoMessages(DEFAULT_INFO);
Review comment:
So, the error message itself is captured in `setErrorMessage`. The `Info Messages` is currently populated with the Exception stacktrace. We should not be sending such internal details to the client. Removing it altogether would be too big of a change, so I just put a static message in there for now as filler.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-784305102
@aihuaxu I posted my concerns and goals in the JIRA ticket. Let me know if you have additional questions:
https://issues.apache.org/jira/browse/HIVE-24772
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-778412685
@aihuaxu @nrg4878
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr merged pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr merged pull request #1974:
URL: https://github.com/apache/hive/pull/1974
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zchovan commented on a change in pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
zchovan commented on a change in pull request #1974:
URL: https://github.com/apache/hive/pull/1974#discussion_r618596419
##########
File path: service/src/test/org/apache/hive/service/cli/TestHiveSQLException.java
##########
@@ -18,13 +18,11 @@
package org.apache.hive.service.cli;
-import java.util.List;
+import java.util.Collections;
Review comment:
Is this import used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] aihuaxu commented on a change in pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
aihuaxu commented on a change in pull request #1974:
URL: https://github.com/apache/hive/pull/1974#discussion_r581260024
##########
File path: service/src/java/org/apache/hive/service/cli/HiveSQLException.java
##########
@@ -102,149 +115,39 @@ public HiveSQLException(String reason, String sqlState, int vendorCode, Throwabl
}
public HiveSQLException(TStatus status) {
- // TODO: set correct vendorCode field
super(status.getErrorMessage(), status.getSqlState(), status.getErrorCode());
- if (status.getInfoMessages() != null) {
- initCause(toCause(status.getInfoMessages()));
- }
}
-
-
-/**
- * Converts current object to a {@link TStatus} object
- * @return a {@link TStatus} object
+ /**
+ * Converts current object to a {@link TStatus} object.
+ *
+ * @return a {@link TStatus} object
*/
public TStatus toTStatus() {
// TODO: convert sqlState, etc.
TStatus tStatus = new TStatus(TStatusCode.ERROR_STATUS);
tStatus.setSqlState(getSQLState());
tStatus.setErrorCode(getErrorCode());
tStatus.setErrorMessage(getMessage());
- tStatus.setInfoMessages(toString(this));
+ tStatus.setInfoMessages(DEFAULT_INFO);
return tStatus;
}
/**
- * Converts the specified {@link Exception} object into a {@link TStatus} object
- * @param e a {@link Exception} object
- * @return a {@link TStatus} object
+ * Converts the specified {@link Exception} object into a {@link TStatus}
+ * object.
+ *
+ * @param e a {@link Exception} object
+ * @return a {@link TStatus} object
*/
public static TStatus toTStatus(Exception e) {
if (e instanceof HiveSQLException) {
- return ((HiveSQLException)e).toTStatus();
+ return ((HiveSQLException) e).toTStatus();
}
TStatus tStatus = new TStatus(TStatusCode.ERROR_STATUS);
tStatus.setErrorMessage(e.getMessage());
- tStatus.setInfoMessages(toString(e));
+ tStatus.setInfoMessages(DEFAULT_INFO);
Review comment:
Make sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-783398846
@zchovan Hello! Great to see you again. Thanks so much for the review. I have made the requested changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-824830468
@miklosgergely @pvary Are you able to review this as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] aihuaxu commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
aihuaxu commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-784390912
Looks good to me. Can you take a look at the test failures?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1974:
URL: https://github.com/apache/hive/pull/1974#discussion_r618760411
##########
File path: service/src/test/org/apache/hive/service/cli/TestHiveSQLException.java
##########
@@ -18,13 +18,11 @@
package org.apache.hive.service.cli;
-import java.util.List;
+import java.util.Collections;
Review comment:
Excellent catch. Will remove and then push. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-778408357
@miklosgergely Supportability improvement
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] aihuaxu commented on pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
aihuaxu commented on pull request #1974:
URL: https://github.com/apache/hive/pull/1974#issuecomment-783535081
@belugabehr Great to meet you here again. :) Can you put more details for ticket description to follow the general requirement and also helps in the future? I will review it as well. Thanks to keep improving it.
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zchovan commented on a change in pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
zchovan commented on a change in pull request #1974:
URL: https://github.com/apache/hive/pull/1974#discussion_r578999876
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -302,7 +301,7 @@ public TRenewDelegationTokenResp RenewDelegationToken(TRenewDelegationTokenReq r
hiveAuthFactory, req.getDelegationToken());
resp.setStatus(OK_STATUS);
} catch (HiveSQLException e) {
- LOG.error("Error obtaining renewing token", e);
+ LOG.error("Failed to renewing token [request: {}]", e);
Review comment:
nit: "Failed to renew" instead of "Failed to renewing"
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -283,7 +282,7 @@ public TCancelDelegationTokenResp CancelDelegationToken(TCancelDelegationTokenRe
hiveAuthFactory, req.getDelegationToken());
resp.setStatus(OK_STATUS);
} catch (HiveSQLException e) {
- LOG.error("Error canceling delegation token", e);
+ LOG.error("Failed to canceling delegation token [request: {}]", req, e);
Review comment:
nit: "Failed to cancel" instead of "Failed to canceling"
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -552,7 +554,7 @@ public TGetInfoResp GetInfo(TGetInfoReq req) throws TException {
resp.setInfoValue(getInfoValue.toTGetInfoValue());
resp.setStatus(OK_STATUS);
} catch (Exception e) {
- LOG.warn("Error getting info: ", e);
+ LOG.warn("Error getting info", e);
Review comment:
should this be error level log? or "Failed to get info"
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -536,7 +538,7 @@ public TCloseSessionResp CloseSession(TCloseSessionReq req) throws TException {
context.setSessionHandle(null);
}
} catch (Exception e) {
- LOG.warn("Error closing session: ", e);
+ LOG.warn("Error closing session", e);
Review comment:
should this be error level log? or maybe change it to "Failed to close the session"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] aihuaxu commented on a change in pull request #1974: HIVE-24772: Revamp Server Request Error Logging
Posted by GitBox <gi...@apache.org>.
aihuaxu commented on a change in pull request #1974:
URL: https://github.com/apache/hive/pull/1974#discussion_r580443156
##########
File path: service/src/java/org/apache/hive/service/cli/HiveSQLException.java
##########
@@ -102,149 +115,39 @@ public HiveSQLException(String reason, String sqlState, int vendorCode, Throwabl
}
public HiveSQLException(TStatus status) {
- // TODO: set correct vendorCode field
super(status.getErrorMessage(), status.getSqlState(), status.getErrorCode());
- if (status.getInfoMessages() != null) {
- initCause(toCause(status.getInfoMessages()));
- }
}
-
-
-/**
- * Converts current object to a {@link TStatus} object
- * @return a {@link TStatus} object
+ /**
+ * Converts current object to a {@link TStatus} object.
+ *
+ * @return a {@link TStatus} object
*/
public TStatus toTStatus() {
// TODO: convert sqlState, etc.
TStatus tStatus = new TStatus(TStatusCode.ERROR_STATUS);
tStatus.setSqlState(getSQLState());
tStatus.setErrorCode(getErrorCode());
tStatus.setErrorMessage(getMessage());
- tStatus.setInfoMessages(toString(this));
+ tStatus.setInfoMessages(DEFAULT_INFO);
return tStatus;
}
/**
- * Converts the specified {@link Exception} object into a {@link TStatus} object
- * @param e a {@link Exception} object
- * @return a {@link TStatus} object
+ * Converts the specified {@link Exception} object into a {@link TStatus}
+ * object.
+ *
+ * @param e a {@link Exception} object
+ * @return a {@link TStatus} object
*/
public static TStatus toTStatus(Exception e) {
if (e instanceof HiveSQLException) {
- return ((HiveSQLException)e).toTStatus();
+ return ((HiveSQLException) e).toTStatus();
}
TStatus tStatus = new TStatus(TStatusCode.ERROR_STATUS);
tStatus.setErrorMessage(e.getMessage());
- tStatus.setInfoMessages(toString(e));
+ tStatus.setInfoMessages(DEFAULT_INFO);
Review comment:
Basically we will throw a general message instead of explicit error message? Is that because we already have such information in ErrorMessage?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org