-
Notifications
You must be signed in to change notification settings - Fork 1.1k
INT-3338: MongoMS: add priority and sequence
#1103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Let me know and I'll go ahead with docs. |
|
Looks ok to me but I don't know mongo 😄 Regarding |
|
Pushed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is original code, but why are these DESCENDING (newest first).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my trick: keep the latest group info and do update only for one document in collection.
And if we don't specify any sort the index is used by default and that one, which includes fields in query. In most cases we use groupIdQuery and it specifies GROUP_ID field. As far as we have only one index which starts with it, that one is selected to get a document for select and update - performance, though!
And that's why I added an important para in docs: to avoid priority/non-priority indexes ambiguity.
If you wish I can add comments here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yes - I hadn't read the documentation 😄
Yes, comments will help (needed) but perhaps we can also detect the improper use and log a WARN? Or, even throw an exception - this message store cannot be used as a MGS and CMS at the same time - select a different collection name for each usage.
...
Maybe could add another method to `PriorityCapableChannelMessageStore
boolean isGroupStoreOperationsPermitted();
and then, when using it as an MGS from correlators etc...
if (store instance of PriorityCapableChannelMessageStore
&& ((PCMS) store).isPriorityEnabled()) {
Assert.isTrue(((PCMS) store).isGroupStoreOperationsPermitted());
}...
Or, maybe best, refactor them so that the CMS is a super class and the full MGS is a subclass. That way, the mistake can't happen.
RedisChannelMessageStore
|-> RedisMessageGroupStore
|
Pushed the solution, when we don't need to worry about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this "only update the last document" trick can work with a priority queue because now the "last" record might get released before other messages and the metadata will be wrong.
Further, surely we really don't need to update this metadata (on the "last" record) when the store is only being used to back a QueueChannel (priority or not), so why take the overhead?
It seems to me that a separate CMS is really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Actually I see now that we can merge both MongoDB to one and fallback to internal MessageReadingMongoConverter, if MappingMongoConverter isn't provided and new useSerialization property is specified as false. Otherwise it needs enough work to support several MSs. According your comment it will be 4 classes.
|
Pushed the new stuff - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?? You decided not to promote this to BasicMessageGroupStore after all ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No-no. It will be here after rebase to my JDBC PR 😄
Add `Sort` for all group queries dependently of `priorityEnabled`. It allow do not configure separate collections for different type of `MessageStore`
|
Pushed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
programlisting within note doesn't render properly - I will move it out of note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh. Need to check docs before push anyway. Thanks
|
Merged |
JIRA: https://jira.spring.io/browse/INT-3338