You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/02 19:25:19 UTC
[GitHub] [druid] TSFenwick opened a new pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
TSFenwick opened a new pull request #11843:
URL: https://github.com/apache/druid/pull/11843
add a class to sanitize JDBC related exceptions and also log them
added a simple test that tests out that the exception gets sanitized
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
Created a simple class to handle logging errors and sanitizing them. This is to be able to sanitize JDBC exceptions.
explored two others ways of solving this.
1) Wrap all the public `DruidMeta` methods in a try catch to sanitize all the JDBC exceptions.
2) Pass a function to handle sanitizing and logging exceptions instead of using a simple class to handle the exceptions.
also added `@NonNull` annotation for `ErrorResponseTransformStrategy` in `ServerConfig`'s constructor.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
##### Key changed/added classes in this PR
* `ErrorHandler`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] been tested in a test Druid cluster.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vtlim commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
vtlim commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r741281339
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
```suggestion
- Druid keeps all checked exceptions as the same exception type except for JDBC-related exceptions and exceptions that are not owned by Druid. Druid labels JDBC-related exceptions as `QueryInterruptedException`. For exceptions that are not originally owned by Druid, Druid applies the transformation strategy to the `errorMessage` field.
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r742398618
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
Move this above `this.throwable = t;`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
@clintropolis should i move this above `this.throwable = t;`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
just log this here since no one is consuming the sanitized throwable? or have this be `this.throwable = errorHandler.logFailureAndSanitize(this.throwable`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11843:
URL: https://github.com/apache/druid/pull/11843#issuecomment-958099455
ugh Travis.... why won't you run!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745926057
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
{
- return logFailureAndSanitize(error, error.getMessage());
+ LOG.error(error, message, format);
+ return error;
}
/**
- * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable at the error level with the throwables message.
*
- * @param error the throwable that will be sanitized
- * @param message the error string formate message to be logged
- * @param format the format args for the message
- * @param <T>
- * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ * @param error the throwable to be logged
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ public static <T extends Throwable> T logFailure(T error)
{
- logFailure(error, message, format);
- return sanitize(error);
- }
-
- public <T extends Throwable> T logFailure(T error, String message, Object... format) {
- LOG.error(error, message, format);
- return error;
- }
-
- public <T extends Throwable> T logFailure(T error) {
logFailure(error, error.getMessage());
return error;
}
+ /**
+ * Sanitizes a Throwable. If it's a runtime exception and it's cause is sanitizable it will sanitize that cause and
+ * return that cause as a sanitized RuntimeException. This will do best effort to keep original exception type. If
+ * it's a checked exception that will be turned into a QueryInterruptedException.
+ * <p>
+ * This was created to sanitize some exceptions that do not need to be logged.
+ *
+ * @param error The Throwable to be sanitized
+ * @param <T> Any class that extends Throwable
+ * @return The sanitized Throwable
+ */
public <T extends Throwable> RuntimeException sanitize(T error)
{
- if (error instanceof QueryInterruptedException) {
- return (QueryInterruptedException) errorResponseTransformStrategy.transformIfNeeded((QueryInterruptedException) error);
+// if (error instanceof NoSuchConnectionException) {
Review comment:
will do
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r743225286
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
dont sanitize here
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
dont sanitize here
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740711307
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
I can't put that inside the `ErrorHandler` because of the method's typing where everything becomes a runtime exception even if there is no error response strategy.
I can out a specific method that preserves this type for the `ErrorHandler`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.NoErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryException;
+import org.apache.druid.query.QueryInterruptedException;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.server.security.ForbiddenException;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ {
+ logFailure(error, message, format);
+ return sanitize(error);
+ }
+
+ public <T extends Throwable> T logFailure(T error, String message, Object... format) {
+ LOG.error(error, message, format);
+ return error;
+ }
+
+ public <T extends Throwable> T logFailure(T error) {
+ logFailure(error, error.getMessage());
+ return error;
+ }
+
+ public <T extends Throwable> RuntimeException sanitize(T error)
+ {
+ if (error instanceof QueryInterruptedException) {
Review comment:
No caller of this throwing a UOE explicitly, but it doesn't hurt to add it imo
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
i'll add more docs to explain this
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745230647
##########
File path: core/src/main/java/org/apache/druid/java/util/common/UOE.java
##########
@@ -19,13 +19,28 @@
package org.apache.druid.java.util.common;
+import com.google.common.base.Strings;
+import org.apache.druid.common.exception.SanitizableException;
+
+import java.util.function.Function;
+
/**
*/
-public class UOE extends UnsupportedOperationException
+public class UOE extends UnsupportedOperationException implements SanitizableException
{
public UOE(String formatText, Object... arguments)
{
super(StringUtils.nonStrictFormat(formatText, arguments));
}
+ @Override
+ public Exception sanitize(Function<String, String> errorMessageTransformFunction)
+ {
+ String transformedErrorMessage = errorMessageTransformFunction.apply(getMessage());
+ if (Strings.isNullOrEmpty(transformedErrorMessage)) {
+ return new ISE("");
Review comment:
Should this be `UOE`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r742409077
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
just log this here since no one is consuming the sanitized throwable? or have this be `this.throwable = errorHandler.logFailureAndSanitize(this.throwable`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740672320
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
`exception to as the same` -> `exception as the same`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
Is it possible to have this logic inside ErrorHandler?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.NoErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryException;
+import org.apache.druid.query.QueryInterruptedException;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.server.security.ForbiddenException;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ {
+ logFailure(error, message, format);
+ return sanitize(error);
+ }
+
+ public <T extends Throwable> T logFailure(T error, String message, Object... format) {
+ LOG.error(error, message, format);
+ return error;
+ }
+
+ public <T extends Throwable> T logFailure(T error) {
+ logFailure(error, error.getMessage());
+ return error;
+ }
+
+ public <T extends Throwable> RuntimeException sanitize(T error)
+ {
+ if (error instanceof QueryInterruptedException) {
Review comment:
Do we need to check for UOE too? Also should UOE implement SanitizableException?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
Ah right.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740712744
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
Ah right.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vtlim commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
vtlim commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r741281339
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
```suggestion
- Druid keeps all checked exceptions as the same exception type except for JDBC-related exceptions and exceptions that are not owned by Druid. Druid labels JDBC-related exceptions as `QueryInterruptedException`. For exceptions that are not originally owned by Druid, Druid applies the transformation strategy to the `errorMessage` field.
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740711307
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
I can't put that inside the `ErrorHandler` because of the method's typing where everything becomes a runtime exception even if there is no error response strategy.
I can out a specific method that preserves this type for the `ErrorHandler`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740676548
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
Is it possible to have this logic inside ErrorHandler?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vtlim commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
vtlim commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r741281339
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
```suggestion
- Druid keeps all checked exceptions as the same exception type except for JDBC-related exceptions and exceptions that are not owned by Druid. Druid labels JDBC-related exceptions as `QueryInterruptedException`. For exceptions that are not originally owned by Druid, Druid applies the transformation strategy to the `errorMessage` field.
```
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
```suggestion
- Druid keeps all checked exceptions as the same exception type except for JDBC-related exceptions and exceptions that are not owned by Druid. Druid labels JDBC-related exceptions as `QueryInterruptedException`. For exceptions that are not originally owned by Druid, Druid applies the transformation strategy to the `errorMessage` field.
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745939057
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
{
- return logFailureAndSanitize(error, error.getMessage());
+ LOG.error(error, message, format);
+ return error;
}
/**
- * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable at the error level with the throwables message.
*
- * @param error the throwable that will be sanitized
- * @param message the error string formate message to be logged
- * @param format the format args for the message
- * @param <T>
- * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ * @param error the throwable to be logged
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ public static <T extends Throwable> T logFailure(T error)
{
- logFailure(error, message, format);
- return sanitize(error);
- }
-
- public <T extends Throwable> T logFailure(T error, String message, Object... format) {
- LOG.error(error, message, format);
- return error;
- }
-
- public <T extends Throwable> T logFailure(T error) {
logFailure(error, error.getMessage());
return error;
}
+ /**
+ * Sanitizes a Throwable. If it's a runtime exception and it's cause is sanitizable it will sanitize that cause and
+ * return that cause as a sanitized RuntimeException. This will do best effort to keep original exception type. If
+ * it's a checked exception that will be turned into a QueryInterruptedException.
+ * <p>
+ * This was created to sanitize some exceptions that do not need to be logged.
+ *
+ * @param error The Throwable to be sanitized
+ * @param <T> Any class that extends Throwable
+ * @return The sanitized Throwable
+ */
public <T extends Throwable> RuntimeException sanitize(T error)
{
- if (error instanceof QueryInterruptedException) {
Review comment:
and pattern of
```java
if (error instanceof Class) {
return (Class) errorResponseTransformStrategy.transformIfNeeded((Class) error)
}
```
doesn't work well if using `QueryInterupptedException` since the Sanitize method that relies on is defined in `QueryException`and that returns a `QueryException` leading to a `java.lang.ClassCastException`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r742398618
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
Move this above `this.throwable = t;`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745958129
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
Review comment:
This was how @clintropolis did it. i reverted my changes to them and just moved them back to `DruidMeta` as they aren't necessary here anymore
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis closed pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
clintropolis closed pull request #11843:
URL: https://github.com/apache/druid/pull/11843
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #11843:
URL: https://github.com/apache/druid/pull/11843
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r737004395
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.initialization.ServerConfig;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error, String message, Object... format)
+ {
+ LOG.error(message, format);
Review comment:
@clintropolis have any thoughts on this?
I didn't change any functionality regarding the logging of the messages here. i just changed what is doing the logging and added the sanitization aspect
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis closed pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
clintropolis closed pull request #11843:
URL: https://github.com/apache/druid/pull/11843
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vtlim commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
vtlim commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r741281339
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
```suggestion
- Druid keeps all checked exceptions as the same exception type except for JDBC-related exceptions and exceptions that are not owned by Druid. Druid labels JDBC-related exceptions as `QueryInterruptedException`. For exceptions that are not originally owned by Druid, Druid applies the transformation strategy to the `errorMessage` field.
```
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
```suggestion
- Druid keeps all checked exceptions as the same exception type except for JDBC-related exceptions and exceptions that are not owned by Druid. Druid labels JDBC-related exceptions as `QueryInterruptedException`. For exceptions that are not originally owned by Druid, Druid applies the transformation strategy to the `errorMessage` field.
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745936632
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
{
- return logFailureAndSanitize(error, error.getMessage());
+ LOG.error(error, message, format);
+ return error;
}
/**
- * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable at the error level with the throwables message.
*
- * @param error the throwable that will be sanitized
- * @param message the error string formate message to be logged
- * @param format the format args for the message
- * @param <T>
- * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ * @param error the throwable to be logged
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ public static <T extends Throwable> T logFailure(T error)
{
- logFailure(error, message, format);
- return sanitize(error);
- }
-
- public <T extends Throwable> T logFailure(T error, String message, Object... format) {
- LOG.error(error, message, format);
- return error;
- }
-
- public <T extends Throwable> T logFailure(T error) {
logFailure(error, error.getMessage());
return error;
}
+ /**
+ * Sanitizes a Throwable. If it's a runtime exception and it's cause is sanitizable it will sanitize that cause and
+ * return that cause as a sanitized RuntimeException. This will do best effort to keep original exception type. If
+ * it's a checked exception that will be turned into a QueryInterruptedException.
+ * <p>
+ * This was created to sanitize some exceptions that do not need to be logged.
+ *
+ * @param error The Throwable to be sanitized
+ * @param <T> Any class that extends Throwable
+ * @return The sanitized Throwable
+ */
public <T extends Throwable> RuntimeException sanitize(T error)
{
- if (error instanceof QueryInterruptedException) {
Review comment:
Because `QueryException` is all that is needed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r737004395
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.initialization.ServerConfig;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error, String message, Object... format)
+ {
+ LOG.error(message, format);
Review comment:
@clintropolis have any thoughts on this?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740672320
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
`exception to as the same` -> `exception as the same`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
Is it possible to have this logic inside ErrorHandler?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.NoErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryException;
+import org.apache.druid.query.QueryInterruptedException;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.server.security.ForbiddenException;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ {
+ logFailure(error, message, format);
+ return sanitize(error);
+ }
+
+ public <T extends Throwable> T logFailure(T error, String message, Object... format) {
+ LOG.error(error, message, format);
+ return error;
+ }
+
+ public <T extends Throwable> T logFailure(T error) {
+ logFailure(error, error.getMessage());
+ return error;
+ }
+
+ public <T extends Throwable> RuntimeException sanitize(T error)
+ {
+ if (error instanceof QueryInterruptedException) {
Review comment:
Do we need to check for UOE too? Also should UOE implement SanitizableException?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
Ah right.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740672320
##########
File path: docs/configuration/index.md
##########
@@ -648,6 +648,7 @@ You can use an error response transform strategy to transform error responses fr
When you specify an error response transform strategy other than `none`, Druid transforms the error responses from Druid services as follows:
- For any query API that fails in the Router service, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
- For any SQL query API that fails, for example `POST /druid/v2/sql/...`, Druid sets the fields `errorClass` and `host` to null. Druid applies the transformation strategy to the `errorMessage` field.
+ - For any JDBC related exceptions, Druid will turn all checked exceptions into QueryInterruptedException otherwise druid will attempt to keep the exception to as the same type, unless original exception isn't owned by druid, Druid applies the transformation strategy to the `errorMessage` field.
Review comment:
`exception to as the same` -> `exception as the same`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740711889
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.NoErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryException;
+import org.apache.druid.query.QueryInterruptedException;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.server.security.ForbiddenException;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ {
+ logFailure(error, message, format);
+ return sanitize(error);
+ }
+
+ public <T extends Throwable> T logFailure(T error, String message, Object... format) {
+ LOG.error(error, message, format);
+ return error;
+ }
+
+ public <T extends Throwable> T logFailure(T error) {
+ logFailure(error, error.getMessage());
+ return error;
+ }
+
+ public <T extends Throwable> RuntimeException sanitize(T error)
+ {
+ if (error instanceof QueryInterruptedException) {
Review comment:
No caller of this throwing a UOE explicitly, but it doesn't hurt to add it imo
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s closed pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
suneet-s closed pull request #11843:
URL: https://github.com/apache/druid/pull/11843
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r737819149
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.initialization.ServerConfig;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error, String message, Object... format)
+ {
+ LOG.error(message, format);
Review comment:
will do
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on pull request #11843:
URL: https://github.com/apache/druid/pull/11843#issuecomment-955020158
Thanks for the contribution. Having good, clean user error messages is great. Even better if they tell the user what's wrong so they know what to do to fix the issue. (SQL error, missing column, unsupported operation, or whatever.)
On the other hand, it is vital that us developers be able to see the error. So, the original, unvarnished, detailed call stack, in all its gory detail, should be logged at some log level.
Example, I make a change an introduce an accidental NPE in my branch. A test fails. I have to be able to see the stack trace to figure out what I botched up so I can fix it.
Does this solution include that logging aspect?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745939743
##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -835,7 +835,7 @@ public void testTooManyConnections() throws Exception
clientNoTrailingSlash.createStatement();
expectedException.expect(AvaticaClientRuntimeException.class);
- expectedException.expectMessage("Too many connections, limit is[4]");
+ expectedException.expectMessage("Too many connections");
Review comment:
The limit is per broker. the extra information was moved into the actual logged messaged
##########
File path: core/src/main/java/org/apache/druid/java/util/common/UOE.java
##########
@@ -19,13 +19,28 @@
package org.apache.druid.java.util.common;
+import com.google.common.base.Strings;
+import org.apache.druid.common.exception.SanitizableException;
+
+import java.util.function.Function;
+
/**
*/
-public class UOE extends UnsupportedOperationException
+public class UOE extends UnsupportedOperationException implements SanitizableException
{
public UOE(String formatText, Object... arguments)
{
super(StringUtils.nonStrictFormat(formatText, arguments));
}
+ @Override
+ public Exception sanitize(Function<String, String> errorMessageTransformFunction)
+ {
+ String transformedErrorMessage = errorMessageTransformFunction.apply(getMessage());
+ if (Strings.isNullOrEmpty(transformedErrorMessage)) {
+ return new ISE("");
+ } else {
+ return new ISE(transformedErrorMessage);
Review comment:
yes it should be
##########
File path: core/src/main/java/org/apache/druid/java/util/common/UOE.java
##########
@@ -19,13 +19,28 @@
package org.apache.druid.java.util.common;
+import com.google.common.base.Strings;
+import org.apache.druid.common.exception.SanitizableException;
+
+import java.util.function.Function;
+
/**
*/
-public class UOE extends UnsupportedOperationException
+public class UOE extends UnsupportedOperationException implements SanitizableException
{
public UOE(String formatText, Object... arguments)
{
super(StringUtils.nonStrictFormat(formatText, arguments));
}
+ @Override
+ public Exception sanitize(Function<String, String> errorMessageTransformFunction)
+ {
+ String transformedErrorMessage = errorMessageTransformFunction.apply(getMessage());
+ if (Strings.isNullOrEmpty(transformedErrorMessage)) {
+ return new ISE("");
Review comment:
yes
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740677336
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.NoErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryException;
+import org.apache.druid.query.QueryInterruptedException;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.server.security.ForbiddenException;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ {
+ logFailure(error, message, format);
+ return sanitize(error);
+ }
+
+ public <T extends Throwable> T logFailure(T error, String message, Object... format) {
+ LOG.error(error, message, format);
+ return error;
+ }
+
+ public <T extends Throwable> T logFailure(T error) {
+ logFailure(error, error.getMessage());
+ return error;
+ }
+
+ public <T extends Throwable> RuntimeException sanitize(T error)
+ {
+ if (error instanceof QueryInterruptedException) {
Review comment:
Do we need to check for UOE too? Also should UOE implement SanitizableException?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r745231766
##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -835,7 +835,7 @@ public void testTooManyConnections() throws Exception
clientNoTrailingSlash.createStatement();
expectedException.expect(AvaticaClientRuntimeException.class);
- expectedException.expectMessage("Too many connections, limit is[4]");
+ expectedException.expectMessage("Too many connections");
Review comment:
why is this changed?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
{
- return logFailureAndSanitize(error, error.getMessage());
+ LOG.error(error, message, format);
+ return error;
}
/**
- * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable at the error level with the throwables message.
*
- * @param error the throwable that will be sanitized
- * @param message the error string formate message to be logged
- * @param format the format args for the message
- * @param <T>
- * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ * @param error the throwable to be logged
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ public static <T extends Throwable> T logFailure(T error)
{
- logFailure(error, message, format);
- return sanitize(error);
- }
-
- public <T extends Throwable> T logFailure(T error, String message, Object... format) {
- LOG.error(error, message, format);
- return error;
- }
-
- public <T extends Throwable> T logFailure(T error) {
logFailure(error, error.getMessage());
return error;
}
+ /**
+ * Sanitizes a Throwable. If it's a runtime exception and it's cause is sanitizable it will sanitize that cause and
+ * return that cause as a sanitized RuntimeException. This will do best effort to keep original exception type. If
+ * it's a checked exception that will be turned into a QueryInterruptedException.
+ * <p>
+ * This was created to sanitize some exceptions that do not need to be logged.
+ *
+ * @param error The Throwable to be sanitized
+ * @param <T> Any class that extends Throwable
+ * @return The sanitized Throwable
+ */
public <T extends Throwable> RuntimeException sanitize(T error)
{
- if (error instanceof QueryInterruptedException) {
Review comment:
Why is the check for QueryInterruptedException removed?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
Review comment:
why do we need logFailure methods instead of just having the caller directly calling LOG.error?
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -47,58 +49,67 @@
}
/**
- * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable and string format message with args at the error level.
*
- * @param error A throwable that will be logged then sanitized
- * @param <T>
- * @return The sanitized throwable
+ * @param error the Throwable to be logged
+ * @param message the message to be logged. Can be in string format structure
+ * @param format the format arguments for the format message string
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ public static <T extends Throwable> T logFailure(T error, String message, Object... format)
{
- return logFailureAndSanitize(error, error.getMessage());
+ LOG.error(error, message, format);
+ return error;
}
/**
- * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
- * the sanitized or original throwable.
+ * Logs any throwable at the error level with the throwables message.
*
- * @param error the throwable that will be sanitized
- * @param message the error string formate message to be logged
- * @param format the format args for the message
- * @param <T>
- * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ * @param error the throwable to be logged
+ * @param <T> any type that extends throwable
+ * @return the original Throwable
*/
- public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ public static <T extends Throwable> T logFailure(T error)
{
- logFailure(error, message, format);
- return sanitize(error);
- }
-
- public <T extends Throwable> T logFailure(T error, String message, Object... format) {
- LOG.error(error, message, format);
- return error;
- }
-
- public <T extends Throwable> T logFailure(T error) {
logFailure(error, error.getMessage());
return error;
}
+ /**
+ * Sanitizes a Throwable. If it's a runtime exception and it's cause is sanitizable it will sanitize that cause and
+ * return that cause as a sanitized RuntimeException. This will do best effort to keep original exception type. If
+ * it's a checked exception that will be turned into a QueryInterruptedException.
+ * <p>
+ * This was created to sanitize some exceptions that do not need to be logged.
+ *
+ * @param error The Throwable to be sanitized
+ * @param <T> Any class that extends Throwable
+ * @return The sanitized Throwable
+ */
public <T extends Throwable> RuntimeException sanitize(T error)
{
- if (error instanceof QueryInterruptedException) {
- return (QueryInterruptedException) errorResponseTransformStrategy.transformIfNeeded((QueryInterruptedException) error);
+// if (error instanceof NoSuchConnectionException) {
Review comment:
Remove commented code if not needed
##########
File path: core/src/main/java/org/apache/druid/java/util/common/UOE.java
##########
@@ -19,13 +19,28 @@
package org.apache.druid.java.util.common;
+import com.google.common.base.Strings;
+import org.apache.druid.common.exception.SanitizableException;
+
+import java.util.function.Function;
+
/**
*/
-public class UOE extends UnsupportedOperationException
+public class UOE extends UnsupportedOperationException implements SanitizableException
{
public UOE(String formatText, Object... arguments)
{
super(StringUtils.nonStrictFormat(formatText, arguments));
}
+ @Override
+ public Exception sanitize(Function<String, String> errorMessageTransformFunction)
+ {
+ String transformedErrorMessage = errorMessageTransformFunction.apply(getMessage());
+ if (Strings.isNullOrEmpty(transformedErrorMessage)) {
+ return new ISE("");
+ } else {
+ return new ISE(transformedErrorMessage);
Review comment:
Should this be `UOE`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r743225286
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
dont sanitize here
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
dont sanitize here
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
dont sanitize here
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
dont sanitize here
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s closed pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
suneet-s closed pull request #11843:
URL: https://github.com/apache/druid/pull/11843
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11843:
URL: https://github.com/apache/druid/pull/11843#issuecomment-958099455
ugh Travis.... why won't you run!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740712217
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
i'll add more docs to explain this
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r742398618
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
@clintropolis should i move this above `this.throwable = t;`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r740711307
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
I can't put that inside the `ErrorHandler` because of the method's typing where everything becomes a runtime exception even if there is no error response strategy.
I can out a specific method that preserves this type for the `ErrorHandler`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.NoErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryException;
+import org.apache.druid.query.QueryInterruptedException;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.server.security.ForbiddenException;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> RuntimeException logFailureAndSanitize(T error, String message, Object... format)
+ {
+ logFailure(error, message, format);
+ return sanitize(error);
+ }
+
+ public <T extends Throwable> T logFailure(T error, String message, Object... format) {
+ LOG.error(error, message, format);
+ return error;
+ }
+
+ public <T extends Throwable> T logFailure(T error) {
+ logFailure(error, error.getMessage());
+ return error;
+ }
+
+ public <T extends Throwable> RuntimeException sanitize(T error)
+ {
+ if (error instanceof QueryInterruptedException) {
Review comment:
No caller of this throwing a UOE explicitly, but it doesn't hurt to add it imo
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
##########
@@ -621,7 +613,12 @@ private DruidConnection getDruidConnection(final String connectionId)
final DruidConnection connection = connections.get(connectionId);
if (connection == null) {
- throw logFailure(new NoSuchConnectionException(connectionId));
+ NoSuchConnectionException noSuchConnectionException = new NoSuchConnectionException(connectionId);
+ // this is to avoid an unecessary cast of NoSuchConnectionException to a runtime exception.
+ if (errorHandler.hasAffectingErrorResponseTransformStrategy()) {
Review comment:
i'll add more docs to explain this
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
TSFenwick commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r742398618
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
Move this above `this.throwable = t;`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
@clintropolis should i move this above `this.throwable = t;`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
just log this here since no one is consuming the sanitized throwable? or have this be `this.throwable = errorHandler.logFailureAndSanitize(this.throwable`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
Move this above `this.throwable = t;`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -390,7 +393,7 @@ private AvaticaParameter createParameter(RelDataTypeField field, RelDataType typ
private DruidStatement closeAndPropagateThrowable(Throwable t)
{
this.throwable = t;
- DruidMeta.logFailure(t);
+ t = errorHandler.logFailureAndSanitize(t);
Review comment:
@clintropolis should i move this above `this.throwable = t;`
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##########
@@ -361,7 +364,7 @@ public void close()
sqlLifecycle.finalizeStateAndEmitLogsAndMetrics(this.throwable, null, -1);
}
} else {
- DruidMeta.logFailure(this.throwable);
+ errorHandler.logFailureAndSanitize(this.throwable);
Review comment:
just log this here since no one is consuming the sanitized throwable? or have this be `this.throwable = errorHandler.logFailureAndSanitize(this.throwable`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers edited a comment on pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
paul-rogers edited a comment on pull request #11843:
URL: https://github.com/apache/druid/pull/11843#issuecomment-955020158
Thanks for the contribution. Having good, clean user error messages is great. Even better if they tell the user what's wrong so they know what to do to fix the issue. (SQL error, missing column, unsupported operation, or whatever.)
On the other hand, it is vital that us developers be able to see the error. So, the original, unvarnished, detailed call stack, in all its gory detail, should be logged at some log level.
Example, I make a change an introduce an accidental NPE in my branch. A test fails. I have to be able to see the stack trace to figure out what I botched up so I can fix it.
Does this solution include that logging aspect?
Edit: checked with the @TSFenwick, turns out we do log the errors, so this aspect is all good.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r736998745
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.initialization.ServerConfig;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error, String message, Object... format)
+ {
+ LOG.error(message, format);
Review comment:
Should we log the stack of the Exception too?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11843: Use a simple class to sanitize JDBC exceptions and also log them
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11843:
URL: https://github.com/apache/druid/pull/11843#discussion_r737792149
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import com.google.inject.Inject;
+import org.apache.druid.common.exception.ErrorResponseTransformStrategy;
+import org.apache.druid.common.exception.SanitizableException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.initialization.ServerConfig;
+
+/**
+ * ErrorHandler is a utilty class that is used to sanitize and log exceptions.
+ */
+class ErrorHandler
+{
+ private static final Logger LOG = new Logger(DruidMeta.class);
+ private final ErrorResponseTransformStrategy errorResponseTransformStrategy;
+
+ @Inject
+ ErrorHandler(final ServerConfig serverConfig)
+ {
+ this.errorResponseTransformStrategy = serverConfig.getErrorResponseTransformStrategy();
+ }
+
+ /**
+ * Logs a throwable at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error A throwable that will be logged then sanitized
+ * @param <T>
+ * @return The sanitized throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error)
+ {
+ return logFailureAndSanitize(error, error.getMessage());
+ }
+
+ /**
+ * Logs an error message at the error level and sanitizes the throwable if applicable. Will return
+ * the sanitized or original throwable.
+ *
+ * @param error the throwable that will be sanitized
+ * @param message the error string formate message to be logged
+ * @param format the format args for the message
+ * @param <T>
+ * @return A sanitized version of the throwable if applicable otherwise the original throwable
+ */
+ public <T extends Throwable> T logFailureAndSanitize(T error, String message, Object... format)
+ {
+ LOG.error(message, format);
Review comment:
i think generally it makes sense to do it as `LOG.error(error, message, format)` so that the exception is attached. Not sure why I didn't do that in the previous change, I guess the only callers were `ForbiddenException` which were thrown directly so probably not a lot of stack there :shrug: Still, though, seems like a reasonable change :+1:
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org