cshannon edited a comment on pull request #756: URL: https://github.com/apache/activemq/pull/756#issuecomment-1030864498
@mattrpav - Overall this is a big improvement over the previous version using getSize(). I verified the sizing and the reported frame sizes when checked on the client side vs server are now quite close (not exact but it's close enough) I see a few things to change: 1. There needs to be tests for all the transports. You need to also test the normal transports (TCP, SSL, etc) and not just NIO. 2. the nio and nio+ssl transports actually check the frame size in a different location on the server than the OpenWireFormat so the NIOSSLTransport and NIOTransport should be updated to also check the new flag. See: https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOSSLTransport.java#L338 and https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOTransport.java#L142 3. I would change the isAssignableFrom() to an instanceof check as it's slightly fast (see the inline comment) 4. You should write edge case tests. For example, we should test all 3 scenarios: frame size is enabled on both client/server, frame size is only enabled on the server and frame size is only enabled on the client. If enabled on the client then we should make sure the connection stays open and if it's a server side only check then the connection should be closed. 5. You should write a test to verify that the framesize flag is not negotiated over open wire to prevent errors in the future or someone changing it. Ie make sure that if either side changes the flag the other side doesn't have it's flag changed. 6. Documentation needs to be added both to the code and to the website that makes it very clear that the flag won't be negotiated and only applies where it is set. https://activemq.apache.org/configuring-wire-formats 7. I think we can probably just rename the flag `verifyMaxFrameSizeEnabled `to just `maxFrameSizeEnabled` . Your comment for the PR also has the wrong name. 8. Lastly I don't think this should be added to 5.16.4 due to it being a change in behavior and a feature. Point releases should really just be about bug fixes and this definitely changes the expected behavior a bit. I think we should only put this into 5.17.0. The flag applies to client/server so we can't put it into 5.16.4 and by default turn it off easily so that's why I think just 5.17.0 -- 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]
