[fix][sql] Fix SQL query is blocked #20911
Conversation
| @@ -443,7 +443,8 @@ public void asyncOpenReadOnlyManagedLedger(String managedLedgerName, | |||
|
|
|||
| }).exceptionally(e -> { | |||
| log.error("[{}] Failed to initialize Read-only managed ledger", managedLedgerName, e); | |||
| callback.openReadOnlyManagedLedgerFailed((ManagedLedgerException) e.getCause(), ctx); | |||
| callback.openReadOnlyManagedLedgerFailed( | |||
| new ManagedLedgerException.ManagedLedgerNotFoundException(e.getMessage()), ctx); | |||
There was a problem hiding this comment.
Why do we convert all of the exceptions to ManagedLedgerNotFoundException?
There was a problem hiding this comment.
It was my oversight, how about changing it to:
callback.openReadOnlyManagedLedgerFailed(
new ManagedLedgerException(e.getMessage()), ctx);
I have tried, but it's rather difficult to reproduce the |
|
@gaoran10 would you mind taking a look at it here? |
|
Do you know why there is |
| @@ -443,7 +443,8 @@ public void asyncOpenReadOnlyManagedLedger(String managedLedgerName, | |||
|
|
|||
| }).exceptionally(e -> { | |||
| log.error("[{}] Failed to initialize Read-only managed ledger", managedLedgerName, e); | |||
| callback.openReadOnlyManagedLedgerFailed((ManagedLedgerException) e.getCause(), ctx); | |||
There was a problem hiding this comment.
Why need this change? Will this cause class cast exception?
There was a problem hiding this comment.
Yes, because this is a PluginClassLoader, it did not directly throw the ClassCastException.
| @@ -328,6 +328,9 @@ Collection<PulsarSplit> getSplitsForTopic(String topicNamePersistenceEncoding, | |||
| splits.add(pulsarSplit); | |||
| } | |||
| return splits; | |||
| } catch (ManagedLedgerException exception){ | |||
There was a problem hiding this comment.
Do you want to catch the case that the managed ledger of the topic is not existing yet? Many exceptions extend ManagedLedgerException, maybe we don't need to catch all ManagedLedgerException.
There was a problem hiding this comment.
If we do not catch exceptions, the client will directly get this exception, but for the user, they do not need to know about this exception, I think we only need to return an empty list.
As for whether to catch all ManagedLedgerException, do you have any suggestions?
After all, the callback function declares ManagedLedgerException and cannot know which specific ManagedLedgerException will be thrown.
Fixes #20910
Motivation
Fix SQL query is blocked.
Modifications
ManagedLedgerExceptioninstead ofe.getCause().Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: crossoverJie#11