GH-8732 Don't remove JDBC message if other groups

[artembilan]

Fixes #8732

When same message is added into different groups,
its record in the INT_MESSAGE must remain until the last group is removed

• Improve JdbcMessageStore.DELETE_MESSAGES_FROM_GROUP SQL to ignore those messages for removal which has other group records in the INT_GROUP_TO_MESSAGE table

Cherry-pick to 6.1.x, 6.0.x & 5.5.x

[garyrussell]

I think we'll need some javadocs on removeMessage(). It is an implementation of the MessageStore method, not a MessageGroupStore method.

Therefore, it will need to override the parent javadoc...

	 * Remove the Message with the given id from the MessageStore, if present, and return it. If no Message with that id
	 * is present in the store, this will return <i>null</i>.

...to indicate that it will not be removed if the message is present in one or more groups.

[garyrussell]

Or, update the parent javadocs to mention that if the implementation is on a MessageGroupStore then...

[garyrussell]

Also, is this just a JDBC problem or can it happen with any MGS?

[artembilan]

or can it happen with any MGS?

I thought about that, but let's see if we are correct about the fix for JDBC only and let's really decide how deep we would like to back-port it!

I believer we have a couple workarounds:

1. Use different MS instances with different region.
2. Don't send the same message to different aggregators with the same MS.

[garyrussell]

Yes, I don't think it was anticipated that the same MGS would be used for different aggregators, and certainly not if those aggregators each get the same message ids.

However, I can see that with large groups (and/or messages), this would be a benefit, avoiding duplication.

[garyrussell]

Perhaps a different issue, but we should consider optimizing addMessage too.

Currently, we detect (and ignore) duplicates after conversion and attempting to update. We might want to check if it already exists first.

[artembilan]

So, I'm OK to have this as a fix only on main and 6.1.x.
Or if you insist I can try to back-port down to 5.5.x as we all agreed that this is a long-standing bug.

However I still in doubts in a query for DELETE_MESSAGES_FROM_GROUP.

Other stores might be fixed in other PR to not make this too complicate.

The addMessage() is indeed a separate issue since it does not donate to the bug and could land just only in the main as an improvement.

[garyrussell]

However I still in doubts in a query ...

?

Do you want it merged, or not?

[artembilan]

Sure! Let's merge it!
But let's cherry-pick only to 6.1.x!
Thanks

[garyrussell]

...and cherry-picked as ccaa853

[artembilan]

@garyrussell ,
regarding addMessage().
I don't think that it is going to be a good optimization to perform one more SELECT before INSERT attempt: it is one more network hope, so not so optimal unlike plain this.serializer.convert(message); in memory.

[artembilan]

Doesn't look like ConfigurableMongoDbMessageStore suffers from the same problem.
The messages are stored in the collection in in a linear way and every single is supplied with groupId.
So, removing one group does not effect others.
We might consider to improve plain removeMessage() which does not consult a presence (not) groupId...

[artembilan]

Looks like an AbstractKeyValueMessageStore has that problem though.
We store plain message ids in the group and every message as a key.
When we remove the group, we just take those ids and iterate them for remove-by-key operation.
Perhaps those message ids in group have to be enhanced with the group id as well: there is no way to have a one-to-many in key-value stores.