ARTEMIS-5573 and ARTEMIS-5975 Improve AMQP Size estimation

[clebertsuconic]

No description provided.

[tabish121]

Overall looks good, nice simplification of some of the message handling

[tabish121]

This was inspired by the proton-j version but isn't a copy and it omits some of the actual validation checks that are done in the proton-j version which are not as thorough as they likely should be but are still better than not validating the encoding at all.

[clebertsuconic]

@tabish121 I don't understand what you're asking me to do here? update the comment?

[clebertsuconic]

@tabish121 this is being called right after the readConstructor was read. the only thing that would be decoded would be either this, or the skipValue() which is doing pretty much the same as this code is doing now.

I'm confused on what are you asking

[tabish121]

I'm saying this does no validation on the data it reads, such as checking that the size value is smaller than the data remaining or on enforcing that the count is actually divisible by two which could indicate that the payload is corrupt if it isn't. I know proton-j does check at least the size data or maybe it checks that count isn't large than the data remaining in the buffer.

My point is that some validation might be sensible here to ensure valid data is being used up front.

[clebertsuconic]

This is doing the same validation thst was being done before.

If you follow the previous call all cobstructor.skipvalue does is read the value and move the bytes.

[clebertsuconic]

I can add the validation though, but this is the same semantic as before.

[tabish121]

Seems like a good place to add a test to validate that an AMQP message with no application properties actually has the correct values for the new size and count value tracking.

[clebertsuconic]

@tabish121 @gemmellr Do you guys know a way to write a NULL encoding, to validate this portion, without having me to deal with bytes directly? I was looking to add one on AMQPDecodeApplicationPropertiesTest.

https://github.com/apache/qpid-proton-j/blob/6dc5587f1d1b23969a8994f1755198e638e92bc4/proton-j/src/main/java/org/apache/qpid/proton/codec/messaging/FastPathApplicationPropertiesType.java#L111-L112

[tabish121]

I'm not sure what null encoding you want to write. I was referring to a message that does not include the section ApplicationProperties at all, meaning it isn't encoded just a message like, Header, MessageAnnotations, Properties and an AmqpValue section

To write an ApplicationProperties that has no actual map payload that would be something like the following

        buffer.writeByte((byte) 0); // Described Type Indicator
        buffer.writeByte(EncodingCodes.SMALLULONG);
        buffer.writeByte(ApplicationProperties.DESCRIPTOR_CODE.byteValue());
        buffer.writeByte(EncodingCodes.NULL);

Proton MessageImpl encoding with an ApplicationProperties object assigned but that has not had a Map added via setValue would probably result in this encoding.

[clebertsuconic]

the codec, apparently support a value type that is empty. basically you can add EncodingCodes.NULL on the encoding and you get an empty ApplicationProperties.

I don't find it very useful but I see it on the codec.

As for the empty application properties I added a test.

[tabish121]

I think we do have code somewhere in the tests that can encode messages with the structure you want, I don't recall where off the top of my head but again, it simply would be assigning an instance ApplicationProperties that has no body assigned to its setValue(Map) element and encoding it which should result in an null as the encoding for that part of the described type.

[clebertsuconic]

I added a commit to be squashed, returning the RELOAD state and persisting the memory estimate on the storage (journal or paging).

I honestly don't think the scan will make any difference.. The real issue is that the estimates were wrong in the first place...

But this commit should settle any discussion.

I think There are already version tests in the Version tests. but i will make sure about it soon.

[clebertsuconic]

for some reason I'm having to use a different persister, and I would have to digg on previous versions to understand why.

I will do some investigation tomorrow, but most likely I will need the V4 persister.

[clebertsuconic]

The reason I had to add a V4 is because of Paging:

On this part here, I encode the number of queues and the queueIDs:

artemis/artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagedMessageImpl.java

Lines 227 to 234 in 5b12277

	int queueIDsSize = buffer.readInt();
	queueIDs = new long[queueIDsSize];
	for (int i = 0; i < queueIDsSize; i++) {
	queueIDs[i] = buffer.readLong();
	}

At the point of the encoder I don't have any reference to the number of bytes used by its own decoder.

I'm adding one on V4 now, if in the future anything else is added, we will stop reading at that marker.

[clebertsuconic]

I intend to squash the second commit on the first as soon some review is done on the new persister.

[clebertsuconic]

I'm setting it as ready to review. but this commit should be squashed before merged.

[clebertsuconic]

I needed to add versioning to AMQPLargeMessagePersister as well.

I could choose to skip memory calculations on large message... however I will prefer to keep it the same way as StandardMessage.

this is also better for future additions