Couple comments on spring-amqp

[Jon Brisbin]

There's several things I think would make using spring-amqp more pleasant. I'm not sure whether this has been geared more toward annotation configuration or not (seems like it is) but there's an inconsistency in the objects that makes working in both a little haphazard.

For example: Exchange can be instantiated with three common args: name, durable, and autoDelete. That's great for annotation configuration but in XML, it's a real pain because <constructor-arg/> means nothing to me when reading the XML. The Spring XML is self-documenting, so I should always be able to specify values on properties and not be forced to use constructor arguments.

Conversely, when using the annotation configuration I would rather have common settings consistent between exchanges and queues. Queue, for example, only takes a single parameter in the constructor. So I have a single line to create an Exchange but three lines of code to construct a Queue.

When constructing a Binding, the Principle of Least Surprise would imply that binding is actually "bound" when fully constructed. But it's just a holder and doesn't actually do anything but encapsulate information about the binding. In fact, I would prefer to have Exchanges, Queues, and Bindings all create themselves when constructed (or at least respond to a boolean saying whether to do this or not). As it is, I have to write quite a few lines of code to do the plumbing (still).

The Binding is another example of inconsistency between annotation configuration and XML configuration. When looking at a Binding constructed in XML, I have no idea what values go where because it relies on <constructor-arg/>. I have to hunt down the javadoc or source to figure that out.

I get confused by the admin's use of the Camel-like syntax because it seems backwards to me. from() takes a Queue. But the logical sequence of events is to have bindings come "from" an exchange "with" a route "to" a queue.

I've seen a dramatic drop in the amount of code AMQP apps demand by employing spring-amqp. Mainly because it has message convertors and other handy tools that remove those requirement from my application code. Kudos on everything so far.

[ndpar]

I agree. All these constructors look ugly

<bean id="topic" class="org.springframework.amqp.core.TopicExchange">
    <constructor-arg value="my_topic" />
    <constructor-arg value="true" />
    <constructor-arg value="false" />
</bean>

<bean id="queue" class="org.springframework.amqp.core.Queue">
    <constructor-arg value="my_queue" />
    <constructor-arg value="true" />
    <constructor-arg value="false" />
    <constructor-arg value="false" />
</bean>

<bean id="binding" class="org.springframework.amqp.core.Binding">
    <constructor-arg ref="queue" />
    <constructor-arg ref="topic" />
    <constructor-arg value="my_key" />
</bean>

<bean id="rabbitAdmin" class="org.springframework.amqp.rabbit.core.RabbitAdmin">
    <constructor-arg ref="rabbitConnectionFactory" />
</bean>

<bean id="rabbitConnectionFactory" class="org.springframework.amqp.rabbit.connection.SingleConnectionFactory"
    p:username="guest" p:password="guest" p:virtualHost="/" p:port="5672">
    <constructor-arg value="localhost" />
</bean>

The configuration should be like this:

<bean id="topic" class="org.springframework.amqp.core.TopicExchange"
    p:name="my_topic"
    p:durable="true"
    p:autoDelete="false" />

<bean id="queue" class="org.springframework.amqp.core.Queue"
    p:name="my_queue"
    p:durable="true"
    p:exclusive="false"
    p:autoDelete="false" />

<bean id="binding" class="org.springframework.amqp.core.Binding"
    p:queue-ref="queue"
    p:exchange-ref="topic"
    p:routingKey="my_key" />

<bean id="rabbitAdmin" class="org.springframework.amqp.rabbit.core.RabbitAdmin"
    p:connectionFactory-ref="rabbitConnectionFactory" />

<bean id="rabbitConnectionFactory" class="org.springframework.amqp.rabbit.connection.SingleConnectionFactory"
    p:username="guest"
    p:password="guest"
    p:virtualHost="/"
    p:port="5672"
    p:host="localhost" />

[Mark Fisher]

There is a big tradeoff there, namely modifying those objects so that they are no longer immutable. Luckily, there is some work being done on a "c" namespace that will look just like the "p" namespace. OTOH, we also have created a fluent API that works well with the @Configuration/@Bean style of configuration, and we definitely encourage people to use that for setting up Exchanges, Queues, and Bindings.

[Jon Brisbin]

To be honest, I don't see a tradeoff at all. When I need objects that act like immutable ones but need to be config'd via Spring, I simply put:

public void setStuff(String s) {
  if(null == s) {
    this.stuff = s;
  }
}

One could add some warning logging if you wanted to if code incorrectly tried to set the value later. Or throw and Exception.

But immutability doesn't seem to me to be an expectation of Spring applications. If we're talking Scala or some other language that actually has real immutability, then I would think that would come into play.

No matter what, nothing changes my dislike of <constructor-arg/> To be honest, I don't really like annotation configuration much, either. I use external values to configure my queue and exchange names. Rather than being able to embed a ${my.queue.name} into a p:name="" reference, I have to have:

  @Value("${mq.host}")
  private String brokerHost;
  @Value("${mq.port}")
  private Integer brokerPort;
  @Value("${mq.user}")
  private String brokerUser;
  @Value("${mq.pass}")
  private String brokerPassword;
  @Value("${mq.vhost}")
  private String brokerVirtualHost;

  @Value("${jobsched.exchange.name}")
  private String exchangeName = "jobsched";

  @Value("${jobsched.sql_queue.name}")
  private String sqlQueueName = "sql";
  @Value("${jobsched.sql_route.name}")
  private String sqlRouteName = "sql.request";

This muddies my configuration classes and, quite frankly, is tedious to manage because every time I add a Queue/Binding to my config, I have to add 4 lines of code.

Forgive me, but I fail to see the improvement over XML configuration.

[Mark Fisher]

Actually, I realized that we do provide setters for all the Exchange types and Queue. I personally would like to change those to be immutable (only after adding namespace support as mentioned below), but now I'm wondering if I misunderstood what the complaints were in this thread?

As far as the setter checking for null, IMO that would need to throw a runtime exception (IllegalStateException or perhaps a more explicit ImmutablePropertyException). Simply returning would suggest that the set had been successful. Don't you think that is dangerous, or at least confusing?
As I alluded to above... we'll be adding namespace support, so that configuring Exchanges and Queues will be simpler via XML:

<queue" name="my_queue" durable="true" exclusive="false" auto-delete="false" />

Obviously that would work well with externalized properties also. Looks good?

Regards,
Mark

[Jon Brisbin]

I'm wondering if I misunderstood what the complaints were in this thread?

I would call them "constructive criticism" rather than complaints.

As far as the setter checking for null, IMO that would need to throw a runtime exception (IllegalStateException or perhaps a more explicit ImmutablePropertyException). Simply returning would suggest that the set had been successful. Don't you think that is dangerous, or at least confusing?

In a library where others are using your code, I'd say throwing an exception is important. In our own stuff, I can do either without much trouble. I tend to err on the side of "let stuff keep going as much as possible". IMHO throwing an exception guarantees an object will be used in the manner intended, where warning logging doesn't break anything but lets the developer know it shouldn't be done how they're trying to do it.

As I alluded to above... we'll be adding namespace support, so that configuring Exchanges and Queues will be simpler via XML:

<queue" name="my_queue" durable="true" exclusive="false" auto-delete="false" />

Obviously that would work well with externalized properties also. Looks good?

Yes, this looks awesome! Can't wait...

[Mark Fisher]

I would call them "constructive criticism" rather than complaints.

Actually, I think it could be classified as "constructor criticism" (ok, bad joke).

Yes, this looks awesome! Can't wait...

Here's the issue: https://jira.springsource.org/browse/AMQP-60

Please provide feedback/suggestions there.

Thanks!
Mark