Use a simple class to sanitize JDBC exceptions and also log them #11843

TSFenwick · 2021-10-25T19:21:44Z

add a class to sanitize JDBC related exceptions and also log them
added a simple test that tests out that the exception gets sanitized

Description

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.

Wrap all the public DruidMeta methods in a try catch to sanitize all the JDBC exceptions.
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.

Key changed/added classes in this PR

ErrorHandler

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

@nonnull

The purpose of this is to sanitize JDBC errors, but can sanitize other errors if they implement SanitizableError Interface add a class to log errors and sanitize them added a simple test that tests out that the error gets sanitized add @nonnull annotation to serverconfig's ErrorResponseTransfromStrategy

…only log specific details This is so an end user gets relevant information but not too much info since they might now how many brokers they have

sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java

maytasm

Are all the exceptions that can be thrown by JDBC implements SanitizableException?

paul-rogers · 2021-10-29T20:11:05Z

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.

added new error types that need to be sanitized also sanitize deprecated and unsupported exceptions.

add docs avoid blanket turning all exceptions into runtime exceptions

docs/configuration/index.md

sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java

sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java

docs/configuration/index.md

suneet-s · 2021-11-02T19:25:00Z

ugh Travis.... why won't you run!

add more javadocs add support UOE sanitization

sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java

…those

core/src/main/java/org/apache/druid/java/util/common/UOE.java

sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java

sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java

clean up bad formatting and commented code. add missed catch for NoSuchStatementException clean up comments for error handler and add comment explainging not wanting to santize avatica exceptions

TSFenwick added 2 commits October 25, 2021 12:16

return less information as part of too many connections, and instead …

6aa2bce

…only log specific details This is so an end user gets relevant information but not too much info since they might now how many brokers they have

asdf2014 added the Area - SQL label Oct 26, 2021

clintropolis closed this Oct 26, 2021

clintropolis reopened this Oct 26, 2021

maytasm reviewed Oct 26, 2021

View reviewed changes

sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java Outdated Show resolved Hide resolved

maytasm requested changes Oct 26, 2021

View reviewed changes

TSFenwick added 2 commits October 29, 2021 13:58

return only runtime exceptions

3729ba6

added new error types that need to be sanitized also sanitize deprecated and unsupported exceptions.

dont reqrewite exceptions unless necessary for checked exceptions

af47ded

add docs avoid blanket turning all exceptions into runtime exceptions

maytasm reviewed Nov 2, 2021

View reviewed changes

docs/configuration/index.md Outdated Show resolved Hide resolved

maytasm reviewed Nov 2, 2021

View reviewed changes

sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java Outdated Show resolved Hide resolved

maytasm reviewed Nov 2, 2021

View reviewed changes

sql/src/main/java/org/apache/druid/sql/avatica/ErrorHandler.java Outdated Show resolved Hide resolved

maytasm approved these changes Nov 2, 2021

View reviewed changes

vtlim reviewed Nov 2, 2021

View reviewed changes

docs/configuration/index.md Outdated Show resolved Hide resolved

suneet-s closed this Nov 2, 2021

suneet-s reopened this Nov 2, 2021

address comments, to fix up docs.

23804fb

add more javadocs add support UOE sanitization

suneet-s closed this Nov 2, 2021

suneet-s reopened this Nov 2, 2021

TSFenwick commented Nov 3, 2021

View reviewed changes

sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java Outdated Show resolved Hide resolved

TSFenwick commented Nov 3, 2021

View reviewed changes

sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java Outdated Show resolved Hide resolved

TSFenwick added 3 commits November 4, 2021 16:28

use try catch instead and sanitize at public methods

c34b04a

checkstyle fixes

3ad4016

throw noSuchStatement and NoSuchConnection as Avatica is affected by …

0ed2e0b

…those

TSFenwick requested a review from maytasm November 8, 2021 18:20

maytasm reviewed Nov 9, 2021

View reviewed changes

core/src/main/java/org/apache/druid/java/util/common/UOE.java Outdated Show resolved Hide resolved

maytasm requested changes Nov 9, 2021

View reviewed changes

address comments. move log error back to druid meta

bba7017

clean up bad formatting and commented code. add missed catch for NoSuchStatementException clean up comments for error handler and add comment explainging not wanting to santize avatica exceptions

maytasm approved these changes Nov 10, 2021

View reviewed changes

clintropolis closed this Nov 10, 2021

clintropolis reopened this Nov 10, 2021

alter test to reflect new error message

f2d0733

suneet-s merged commit 1487f55 into apache:master Nov 16, 2021

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request May 25, 2022

[Draft] 0.23.0 Release notes #12510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a simple class to sanitize JDBC exceptions and also log them #11843

Use a simple class to sanitize JDBC exceptions and also log them #11843

TSFenwick commented Oct 25, 2021

maytasm left a comment

paul-rogers commented Oct 29, 2021 •

edited

Loading

suneet-s commented Nov 2, 2021

Use a simple class to sanitize JDBC exceptions and also log them #11843

Use a simple class to sanitize JDBC exceptions and also log them #11843

Conversation

TSFenwick commented Oct 25, 2021

Description

Key changed/added classes in this PR

maytasm left a comment

Choose a reason for hiding this comment

paul-rogers commented Oct 29, 2021 • edited Loading

suneet-s commented Nov 2, 2021

paul-rogers commented Oct 29, 2021 •

edited

Loading