gaoran10 commented on code in PR #20911:
URL: https://github.com/apache/pulsar/pull/20911#discussion_r1328180420
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java:
##########
@@ -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(e.getMessage()), ctx);
Review Comment:
Maybe you should use the method
`ManagedLedgerException#getManagedLedgerException`.
##########
pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSplitManager.java:
##########
@@ -328,6 +328,9 @@ Collection<PulsarSplit> getSplitsForTopic(String
topicNamePersistenceEncoding,
splits.add(pulsarSplit);
}
return splits;
+ } catch (ManagedLedgerException exception){
Review Comment:
Could you reproduce this problem? I don't know the root cause of the
`NoSuchElementException`, it's hard to judge whether need to throw a server
exception to clients, if this is a normal case, we don't need to notify
clients, if this is abnormal, I think we need to send a server exception to
clients, or else users will be confused by the empty result.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
