Skip to content
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

WIP: [AMQ-9455] DestinationPolicy support for MessageStrategy #1182

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattrpav
Copy link
Contributor

No description provided.

@mattrpav mattrpav self-assigned this Mar 18, 2024
@mattrpav mattrpav changed the title [AMQ-9455] MessageStrategy policy handler WIP: [AMQ-9455] MessageStrategy policy handler Mar 18, 2024
@mattrpav mattrpav force-pushed the AMQ-9455 branch 3 times, most recently from 960c8d8 to bbe2dac Compare April 30, 2024 01:22
@mattrpav mattrpav changed the title WIP: [AMQ-9455] MessageStrategy policy handler WIP: [AMQ-9455] DestinationPolicy support for MessageStrategy Apr 30, 2024
@cshannon
Copy link
Contributor

It's not really clear to me when I first read this what MessageStrategy was supposed to be from the name so I'm wondering if we should rename it. if this is supposed to be essentially an interceptor framework to mutate a message the maybe we just call it something like MessageInterceptor. However, I know we used the term "Strategy" elsewhere

Also, the API may need some discussion. A lot of times with something like this you might want to return the message so you can chain it and actually return an entirely different object, but this could have its own problems with things like potential memory usage not being tracked correctly. This only supports mutating in place which can have problems as well.

Anyways, the main thing is just making sure the public API is well thought out, named well, decide if we want to allow mutating in place only or not, etc because once it's out there it's hard to change it as people will create implementations.

@mattrpav
Copy link
Contributor Author

mattrpav commented May 29, 2024

@cshannon agreed, great points. +1 on returning the object in the API.

  1. Re naming-- there is already a MessageInterceptor.java that is broker-wide, so I went with 'Strategy' since it matches the other sub-items on PolicyEntry -- SlowConsumerStrategy, DeadLetterStrategy, etc. That being said, I'm not married to 'MessageStrategy' as a name in any way.

  2. Great point re memory usage and mutating. I think a key decision is if we have some sort of core MessageStrategy handler that supports memoryUsage on behalf of the extensions, or if each extension is responsible for managing that. I think having a top-level handler would be better, so users could do simple things like 'require this header name' and not worry about queue internals like memoryUsage. The ChainMessageStrategy could do that handling so users could add 'simple' extensions as children to that or replace the strategy altogether and be responsible for memoryUsage and other queue internals themselves. Thoughts?

@cshannon
Copy link
Contributor

cshannon commented May 29, 2024

So I double checked and yeah we pretty much just used "Strategy" for everything. So maybe we could make it more descriptive by adding an extra word to the name that describes it better like Message<something>Strategy instead of just MessageStrategy to convey what it is. Not sure... maybe MessageInterceptorStrategy, MessageEnricherStrategy, MessageMutatorStrategy, etc. I'm open for ideas

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