Skip to content

Conversation

garyrussell
Copy link
Contributor

JIRA: https://jira.spring.io/browse/INT-3325

Optimized MGS for QueueChannel - uses a LIST for
each channel and LPUSH, RPOP.

  • Also fix MutableMessage to be Serializable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems for me like we should introduce new marker interface ChannelMessageStore and protect QueueChannel with that and disallow to use other out of the box implementations.
WDYT?

Otherwise, of course, LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - and maybe a sub-interface PriorityChannelMessageStore which would be the required type if we open up message-store on priority-channel (and you'd get a QueueChannel with a PriorityChannelMessageStore instead of a PriorityChannel).

I think disallowing general message stores might be a little too much; some use cases might be just fine and it would be a burden for us to force them to maintain 2 sets of, say, JDBC tables. Maybe we could just emit a WARN for now if they don't use an optimized store (or just leave it to the documentation).

I will rework this PR on Monday.

@garyrussell
Copy link
Contributor Author

Added marker interfaces, priority store.

Rebased, squashed, pushed.

JIRA: https://jira.spring.io/browse/INT-3325
JIRA: https://jira.spring.io/browse/INT-1870

Optimized MGS for QueueChannel - uses a LIST for
each channel and LPUSH, RPOP.

* Also fix MutableMessage to be Serializable

INT-1870 Priority Redis Channel Message Store

Supports priorities 0-9 (+ no priority).

Priorities out of that range are treated as no priority.

Polishing - Add Marker Interfaces

* Emit a `WARN` log if a channel is used with a regular MessageGroupStore
* Allow message-store on namespace when defining a priority channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, ChannelMessageStore

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BasicMessageGroupStore is redundant

@artembilan
Copy link
Member

No polishing yet, but the main purpose to coerce like this RedisChannelMessageStore:

private final RedisTemplate<Object, Message<?>> redisTemplate;

And I see that here is a confuse around this.getBeanName() + ":".
I understand the purpose of it - the region to allow to use several MS on the same Redis: 👍
But we should be carefull how we do it: in some place you use it, but in others don't: just provide the full groupId from test, but I'm not sure that QueueChannel on addMessageToGroup provide the region too.

@artembilan
Copy link
Member

Onother thought.
May we anyway to encapsulate all option within the single MS implementation and allow to end-user to use it from any message-store option? And determine which type of operation to perform at runtime, e.g. via some ThreadLocal variable. Of course here we have to be carefull around this.getBeanName() + ":*" for managed operation. However we can't provide ThreadLocal from JMX to separate the operation for queue and for priority-queue. Although we have the same for JDBC (region and several channels) and it doesn't cause any confuse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant

@garyrussell
Copy link
Contributor Author

May we anyway to encapsulate all option within the single MS implementation...

I prefer to keep separate implementations - you will see that every "business" method from the superclass is overridden; they just share infrastructure methods. That means we'd have an if test in every method, raising the complexity. It seems cleaner to me to keep them separate - also, it makes it easier to do the assertions in MGQ.

@artembilan
Copy link
Member

OK. Nevermind: canceled the encapsulation question.
Let's go ahead with other concerns here!

@garyrussell
Copy link
Contributor Author

Pushed

@artembilan
Copy link
Member

LGTM
Merging with simple red JavaDocs and some typos in Docs

@artembilan
Copy link
Member

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants