gemmellr commented on code in PR #4430:
URL: https://github.com/apache/activemq-artemis/pull/4430#discussion_r1161638521
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java:
##########
@@ -1130,14 +1130,16 @@ private void sendContinuations(final int packetSize,
try {
session.send(session.getCurrentTransaction(),
EmbedMessageUtil.extractEmbedded((ICoreMessage) message.toMessage(),
storageManager), false, producers.get(senderID), false);
- } catch (Exception e) {
- message.deleteFile();
- throw e;
} catch (Throwable e) {
-
logger.warn("********************************************************************************");
- logger.warn("Throwable on currentLargeMessage {}",
message.getMessageID(), e);
-
logger.warn("********************************************************************************");
-
+ logger.warn(e.getMessage(), e);
Review Comment:
Putting this here and removing the 'catch(Exception e)' means it will begin
printing warnings with stacktraces for _every_ exception thrown in send now, vs
none before, even though its still throwing the exception onward, to someplace
that may also print it if not handling it specifically. Its not clear to me
thats desirable now if it was specifically avoided before, I expect it to
generate a bunch of stacktraces people will then report as 'new errors' as they
arent used to it.
Previously it only logged warning for the 'unhandled' Error's it then
swallowed. You've changed it to rethrow those too. Perhaps just make a debug
log for the Exceptions if wanting to add potential visibility, and retain the
warning level for the Throwables?
Using 't' is more common for throwables than 'e'.
--
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]
