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] JmsMessageDrivenChannelAdapterDefaultListenerContainerSpec #2969

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@agebhar1
Copy link
Contributor

commented Jun 19, 2019

Create class for concrete implementation of JmsMessageDrivenChannelAdapterListenerContainerSpec<JmsDefaultListenerContainerSpec, DefaultMessageListenerContainer> to avoid usage of JmsMessageDrivenChannelAdapterListenerContainerSpec#configureListenerContainer(...) and expose all (?) methods of JmsDefaultListenerContainerSpec for convenience.

@artembilan

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

So, and what is wrong with the configureListenerContainer()?
With such a new class we have to support additional generic argument to not have a copy/paste of all the JmsMessageDrivenChannelAdapterListenerContainerSpec method in your new class.
And at the same time we need to get rid of that configureListenerContainer() in your new class somehow. What is not possible and going to be confusing for end-users.
IMHO we don't need this extra class and JmsMessageDrivenChannelAdapterListenerContainerSpec with its configureListenerContainer() covers us with any possible requirements.

May you elaborate more, please?

@agebhar1

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

I'll try it. There's totally nothing wrong with configureListenerContainer. While typing the integration flow by Java DSL for (JMS) Message Driven Channel Adapter with a connection factory a more specific auto-completion by the IDE might help to explore the possible configuration due to named methods.

To be concrete. It took some time to figure out that I need to set the platform transaction manager to use distributed transaction with the provided (JMS) connection factory.

I am aware that copy/paste is awkward.

@artembilan
Copy link
Member

left a comment

Can you just go with the messageDrivenChannelAdapter(JmsListenerContainerSpec<?, ? extends AbstractMessageListenerContainer> jmsListenerContainerSpec) variant and use Jms.container()?

I mean your concern is valid, but there are a lot of to support in that new class when we are going to change underlying API. I even wonder if we should deprecate an existing JmsMessageDrivenChannelAdapterListenerContainerSpec in favor of the mentioned Jms.container() top-level API...

@agebhar1

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Can you just go with the messageDrivenChannelAdapter(JmsListenerContainerSpec<?, ? extends AbstractMessageListenerContainer> jmsListenerContainerSpec) variant and use Jms.container()?

Sure.

I even wonder if we should deprecate an existing JmsMessageDrivenChannelAdapterListenerContainerSpec in favor of the mentioned Jms.container() top-level API...

It might be a consequence since there is only destination(...) delegated to the Spec of JmsMessageDrivenChannelAdapterListenerContainerSpec<S ... other methods such errorHandler(...) are already absent.

@artembilan

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

So, do we agree to reconsider an introduction of a new class in favor of deprecating an existing one?
Just because there is already Jms.container() API which covers all the requirements.

We indeed may miss some options there in the JmsListenerContainerSpec and its extension, so this should be a good opportunity to review that part as well.
Without and intermediate JmsMessageDrivenChannelAdapterListenerContainerSpec it should become much easier to support future changes in JMS components.

WDYT?

@agebhar1

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Yes, I think so - we agree.

FTR Jms.container(ConnectionFactory connectionFactory, ...) is the equivalent to Jms.messageDrivenChannelAdapter(ConnectionFactory connectionFactory).destination(...).configureListenerContainer(container -> {...})

I used the Jms.messageDrivenChannelAdapter(...) due to the notice given by JMS Transaction doc

Alternatively, consider using a jms-message-driven-channel-adapter with acknowledge set to transacted (the default).

Maybe a slightly better name for Jms.container(...) should help too, since the container which is used is DefaultMessageListenerContainer.class.

If destination(...) might be public:

public class JmsListenerContainerSpec< ... > {

  public S destination(...) {
     ...
  }

}

one can write

Jms.container(...).destination(...). ...
@artembilan

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Well, the point of such a container(ConnectionFactory connectionFactory, Destination destination) signature that destination is a required option:

@Override
	protected void validateConfiguration() {
		if (this.destination == null) {
			throw new IllegalArgumentException("Property 'destination' or 'destinationName' is required");
		}
	}

So, my idea with such an API to enforce end-user to not miss this important option.
This is some code style rule in our project we try to follow:

If option is required, it must be requested from the class constructor, so end-user won't miss to set it and won't deal with unexpected exception on first project pass

The same definitely is applied for the factory methods.

Not sure why you worry about that container() factory name since it should be self-explanatory: The factory class is Jms. it has that container() method. Therefore it is indeed about a MessageListenerContainer implementation.
The return of that method is JmsDefaultListenerContainerSpec. There are JavaDocs explaining this factory method, as well as a JmsDefaultListenerContainerSpec has JavaDocs as well.
Not sure what else should be more specific...

The Jms.messageDrivenChannelAdapter() doesn't require a destination because it can be configured with the destinationName instead.
Well, we can revise that as well into two factory methods similar to the container() variants, if you feel strong.
But we need to be ready to support several API for backward compatibility, since an existing one should be deprecated to avoid confusion.

I think you need to play with this API a little more to get used to it.
We indeed did a hard work designing it for convenience and flexibility.
I don't mind that we may miss something, but in most cases there is the way do not brake the code.

Hope it is clear.

@agebhar1

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

@artembilan thanks a lot for the helpful insights.

@agebhar1 agebhar1 closed this Jun 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.