Fix KV Stores for same message in multiple groups #8737
Conversation
|
Hold off from merging yet, please. |
artembilan
force-pushed
the
Fix_KeyValueStore_for_multi_groups
branch
from
33dcd17 to
8543919
Compare
|
Now this is good for review, merge and cherry-pick. Thanks |
...n-core/src/main/java/org/springframework/integration/store/AbstractKeyValueMessageStore.java
Show resolved
Hide resolved
| @@ -161,11 +167,10 @@ public Message<?> removeMessage(UUID id) { | |||
| @Override | |||
| @ManagedAttribute | |||
| public long getMessageCount() { | |||
| Collection<?> messageIds = doListKeys(this.messagePrefix + '*'); | |||
| Collection<?> messageIds = doListKeys(this.messagePrefix + "[^GROUP_]*"); | |||
There was a problem hiding this comment.
It's not clear how this works; can you explain further?
There was a problem hiding this comment.
We have two prefixes: MESSAGE_ and MESSAGE_GROUP_.
When we do request for MESSAGE_ pattern, the result also includes entries for groups.
Which we don't want to have when we only count message entries.
The new pattern works this way (by default): MESSAGE_[^GROUP_]*. So, include those keys which starts from MESSAGE_, but don't follow with the GROUP_ sub-prefix.
Does it makes sense now?
Borrowed from here: https://redis.io/commands/keys/.
And it turns out to be similar to regexp, so we can reuse this pattern for similar fix in Hazelcast store impl.
There was a problem hiding this comment.
But, with regex, [^GROUP_] means match any string that does not contain any of those characters (not that sequence of characters); I would expect the same semantics from Redis.
Also, for regex, it must be .* to match all remaining chars (but I do see the change to .+ in Hazelcast).
...
But, it does seem to work, at least with Redis; I just connected to the test container with a breakpoint...
% redis-cli -p 61866
127.0.0.1:61866> keys *
1) "MESSAGE_GROUP_1"
2) "MESSAGE_1_b487eb34-5e29-6dcc-abeb-ffdcc70f7bf8"
3) "MESSAGE_2_b487eb34-5e29-6dcc-abeb-ffdcc70f7bf8"
4) "MESSAGE_GROUP_2"
127.0.0.1:61866> keys 'MESSAGE_[^GROUP_]*'
1) "MESSAGE_1_b487eb34-5e29-6dcc-abeb-ffdcc70f7bf8"
2) "MESSAGE_2_b487eb34-5e29-6dcc-abeb-ffdcc70f7bf8"
See this for an explanation of how to find matches that don't contain a sequence of chars.
There was a problem hiding this comment.
Note, however, that if the user's group is GROUP_1 (instead of just 1) the query would fail.
Also, FYI...
127.0.0.1:62123> keys 'MESSAGE_[^G]*'
1) "MESSAGE_2_28b71e31-8d30-0725-bad8-96e5a56925c6"
2) "MESSAGE_1_28b71e31-8d30-0725-bad8-96e5a56925c6"
works too, but that would be even worse; any group starting with G would fail to be found.
There was a problem hiding this comment.
I think Redis pattern is not fully regexp, therefore that one.
but according to your findings to make it working properly with Hazelcast we have to do this:
MESSAGE_(?!GROUP_).*
Correct?
There was a problem hiding this comment.
OK. So, to make it more robust we indeed have to rename that prefix for groups. 🤷
Is that what you trying to say?
There was a problem hiding this comment.
Correct?
Yes, I think so, according to that answer.
Is that what you trying to say?
Yes, test fails (0 messages) if I change it to
store.addMessageToGroup("GROUP_1", testMessage);
store.addMessageToGroup("GROUP_2", testMessage);There was a problem hiding this comment.
Just another FYI; this
store.addMessageToGroup("ONE", testMessage);
Fails too (because of the [^O] in [^GROUP_]).
127.0.0.1:63751> keys 'MESSAGE_[^GROUP_]*'
1) "MESSAGE_TWO_f4c2d730-c8ce-f955-d8e0-f54710639264"
127.0.0.1:63751> keys *
1) "MESSAGE_GROUP_TWO"
2) "MESSAGE_GROUP_ONE"
3) "MESSAGE_ONE_f4c2d730-c8ce-f955-d8e0-f54710639264"
4) "MESSAGE_TWO_f4c2d730-c8ce-f955-d8e0-f54710639264"
If same message is stored into different groups with the same KV store, the removal of one group would lead to removal the message for the other one. * Improve KV Store to save message with the key including a group id * Respectively, refine the removal API to include group id into keys * Also change the `MESSAGE_GROUP_KEY_PREFIX` for group records to `GROUP_OF_MESSAGES_` since the `MESSAGE_` prefix includes group records as well for various operations based on key pattern
**Cherry-pick to `6.1.x`**
to avoid complex regexp and don't bother for edge cases, where even that regexp may fail
artembilan
force-pushed
the
Fix_KeyValueStore_for_multi_groups
branch
from
bdca88a to
cdcdecc
Compare
|
OK. I brought back a |
If same message is stored into different groups with the same KV store, the removal of one group would lead to removal the message for the other one.
MESSAGE_GROUP_KEY_PREFIXfor group records toGROUP_OF_MESSAGES_since theMESSAGE_prefix includes group records as well for various operations based on key patternThe problem was spotted in JDBC: #8732
This is a breaking change not only for group prefix, but for key pattern for records in the store.
Therefore this is not recommended for back-porting: the respective Migration Guide note will be added after merge.
The workaround is similar to one proposed for JDBC: don't store same message to different groups; don't use same store instance in different components.
The
MESSAGE_*pattern is not fixable though with any workarounds: thegetMessageCount()may provide false information.