You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "clebertsuconic (via GitHub)" <gi...@apache.org> on 2023/04/07 16:25:59 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request, #4430: ARTEMIS-4171/ARTEMIS-4233 Large Message Send
clebertsuconic opened a new pull request, #4430:
URL: https://github.com/apache/activemq-artemis/pull/4430
This is not really supposed to happen but I'm dealing with it just in case. In case it happened I would like to see the message removed and an exception thrown to the client
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4430: ARTEMIS-4171/ARTEMIS-4233 Large Message Send
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4430: ARTEMIS-4171/ARTEMIS-4233 Large Message Send
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4430:
URL: https://github.com/apache/activemq-artemis/pull/4430#discussion_r1162809029
##########
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:
I thought the system was not really supposed to throw an exception at this stage, and logging it would be only beneficial to finding what's going in a production system.
I'm dealing with other cases where it's being hard to identify issues, and I was trying to avoid that pattern here.
One thing is sending an expected exception to the client... (expected as it's something related to the client's configuration...etc..) the other is a server's exception not logging anything and only the client being informed about it.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic closed pull request #4430: ARTEMIS-4171/ARTEMIS-4233 Large Message Send
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic closed pull request #4430: ARTEMIS-4171/ARTEMIS-4233 Large Message Send
URL: https://github.com/apache/activemq-artemis/pull/4430
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4430: ARTEMIS-4171/ARTEMIS-4233 Large Message Send
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4430:
URL: https://github.com/apache/activemq-artemis/pull/4430#discussion_r1162887459
##########
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:
The code this method is calling looks to throw exceptions in fairly 'typical' situations, e.g security checks, disk usage checks. I think starting to log a full stacktrace for every one of them, when it specifically didnt before (presumably as the code calling this method should either handle, or itself print, them), is likely asking for a lot of new questions. Along the lines of the more typical 'I suddenly got this new error \<provides non-error info log message and/or stacktrace\>'.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org