You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2023/05/17 04:32:11 UTC
[GitHub] [druid] kfaraz commented on a diff in pull request #14271: Do not retry SQL operation in case of `max_allowed_packet` exception
kfaraz commented on code in PR #14271:
URL: https://github.com/apache/druid/pull/14271#discussion_r1195918488
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -165,14 +166,21 @@ public <T> T retryTransaction(final TransactionCallback<T> callback, final int q
public final boolean isTransientException(Throwable e)
{
- return e != null && (e instanceof RetryTransactionException
- || e instanceof SQLTransientException
- || e instanceof SQLRecoverableException
- || e instanceof UnableToObtainConnectionException
- || e instanceof UnableToExecuteStatementException
- || connectorIsTransientException(e)
- || (e instanceof SQLException && isTransientException(e.getCause()))
- || (e instanceof DBIException && isTransientException(e.getCause())));
+ if (e == null) {
+ return false;
+ }
+ if (e.getMessage() != null && e.getMessage().contains(MAX_ALLOWED_PACKET_ERROR)) {
Review Comment:
For MySQL, the SQLState seems to be `S1000`, error code is `0`, and the root cause exception is `PacketTooBigException`.
```
org.skife.jdbi.v2.exceptions.CallbackFailedException: org.skife.jdbi.v2.exceptions.UnableToExecuteStatementException: com.mysql.jdbc.PacketTooBigException: Packet for query is too large (9952 > 1024). You can change this value on the server by setting the max_allowed_packet' variable.
```
The solution for this case is simply to check if the cause of `UnableToExecuteStatementException`, which we should always do anyway because `UnableToExecuteStatementException` is not transient by itself. Since `PacketTooBigException` is not transient by any of our other rules, it would be categorized correctly.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -165,14 +166,21 @@ public <T> T retryTransaction(final TransactionCallback<T> callback, final int q
public final boolean isTransientException(Throwable e)
{
- return e != null && (e instanceof RetryTransactionException
- || e instanceof SQLTransientException
- || e instanceof SQLRecoverableException
- || e instanceof UnableToObtainConnectionException
- || e instanceof UnableToExecuteStatementException
- || connectorIsTransientException(e)
- || (e instanceof SQLException && isTransientException(e.getCause()))
- || (e instanceof DBIException && isTransientException(e.getCause())));
+ if (e == null) {
+ return false;
+ }
+ if (e.getMessage() != null && e.getMessage().contains(MAX_ALLOWED_PACKET_ERROR)) {
Review Comment:
Unfortunately, this is not the exception we had seen in prod. I tried reproducing it by keeping a high `max_allowed_packet` on the mysql server but a low value in the connection url, but that that again resulted in the same `PacketTooLargeException`. In prod, we had seen a nesting of `CallbackFailedException` -> `UnableToExecuteStatementException` -> `SQLTransientConnectionException`.
--
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