[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3079: ARTEMIS-2706 Use FrameSize to decide when to flush large messages

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3079: ARTEMIS-2706 Use FrameSize to decide when to flush large messages
URL: https://github.com/apache/activemq-artemis/pull/3079#discussion_r408038172
 
 

 ##########
 File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
 ##########
 @@ -862,7 +862,9 @@ void deliver() {
 
                   sender.send(buf.array(), 0, size);
 
-                  connection.instantFlush();
+                  if (protonSession.session.getOutgoingBytes() >= protonSession.session.getConnection().getTransport().getOutboundFrameSizeLimit()) {
+                     connection.instantFlush();
+                  }
 
 Review comment:
   I dont think this makes the most sense as there isnt anything to say there arent bytes for other messages in the session, so using that to govern behaviour for a particular delivery isnt a reliable way to achieve the desired behaviour.
   
   The delivery has its own buffer, using that to govern behaviour would seem the most appropriate, see Delivery.pending() and available() (which both do the same thing hehe, https://issues.apache.org/jira/browse/PROTON-1409).
   
   Just checking the brokers outbound size limit doesnt allow for the fact the receivers frame size may be lower than it, so it may be worth calculating the lower of the two, otherwise it may never flush here. Similarly, the return slightly higher up may mean it returns without flushing.
   
   The flush here will presumably also be why ARTEMIS-2707 occurred, since the delivery isnt indicated complete (sender.advance()) or settled (which implicitly completes) until further down just before another flush, yet would have already had all its payload sent at this point. With the addition of an 'if outstanding is over frame size' guard here it would become less certain whether the flush and subsquent empty transfer occurred however, depending then on whether the threshhold was exceeded after the last send.
   
   I'd note that by creating such a small buffer, you are forcing a similarly small allocation on every send call as the payload is copied into the delivery. Using a bigger buffer would seem sensible when appropriate. Perhaps the message size can be discerned and something suitable selected based on that and the frame sizing. Or also using NettyReadable and sendNoCopy to avoid the copy.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services