gemmellr commented on issue #2490: V2 196 URL: https://github.com/apache/activemq-artemis/pull/2490#issuecomment-453121357 @michaelandrepearce I'd have made the same comment for those changes if reviewing them, as I would probably have to go digging through commits now if I wanted to understand the evolution there for subsequent changes, when at least a simple comment might have done the trick. Giving up on this point though, enough time wasted looking to make things clearer for later readers to avoid people needing to waste time :) Its more the other side of the comment that primarily concerns me, that the new client always sends the extra detail to old servers despite knowing they cant use it. The fact the old server then doesnt read out the extra info in the buffer is what I find most concerning, as the data is going to be there in the buffer, and so how that buffer is later used by the old server becomes important following the changes even though it doesnt support the feature. The basic compatibility tests dont really ease that concern. It just seems simpler and safer to ensure ongoing compatibility isnt affected by not sending old servers new data we know they cant use, but could perhaps trip over in unexpected ways. Looking at the existing code now rather than just the diff here, it seems the very first thing it does after decoding is use the current buffer index as a 'size', which then might not be accurate any more depending on how its being used or how it interacts with other areas later. I haven't traced further usage of the buffer or that size, but I personally wouldn't be comfortable with the new data being sent to old servers based on that alone. Who knows what things other areas and even older different versions of the code might do in the face of changing the buffer content.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
