cshannon commented on code in PR #2071:
URL: https://github.com/apache/activemq/pull/2071#discussion_r3352340622
##########
activemq-broker/src/main/java/org/apache/activemq/advisory/AdvisoryBroker.java:
##########
@@ -1030,4 +1031,50 @@ private AdvisoryBroker getOuterType() {
return AdvisoryBroker.this;
}
}
+
+ protected ProducerBrokerExchange newAdvisoryProducerExchange() {
+ final ProducerBrokerExchange producerExchange = new
ProducerBrokerExchange();
+
producerExchange.setConnectionContext(Objects.requireNonNull(advisoryConnectionContext.get(),
+ "Advisory ConnectionContext must not be null"));
+ producerExchange.setMutable(true);
+ producerExchange.setProducerState(new ProducerState(new
ProducerInfo()));
+ return producerExchange;
+ }
+
+ // Lazy load becuase we need to call
getBrokerService().getAdminConnectionContext()
+ // after the constructor finishes to allow the Broker chain to finish
initializing
+ // to prevent a stack overflow. Uses double-checked locking abstracted away
+ // to share the advisory admin context to load on demand.
+ protected class AdvisoryConnContextSupplier implements
Supplier<ConnectionContext> {
+
+ private volatile ConnectionContext advisoryContext;
+
+ @Override
+ public ConnectionContext get() {
+ ConnectionContext result = advisoryContext;
+
+ if (result == null) {
+ synchronized (this) {
+ result = advisoryContext;
+ if (result == null) {
+ try {
+ // Copy so we can set flow control false
+ advisoryContext = result =
+
getBrokerService().getAdminConnectionContext().copy();
+ // We never want to use flow control for advisories
+ result.setProducerFlowControl(false);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
Review Comment:
The idea here is we are lazy loading one time and re-using the context so
this wouldn't be done every time, although I guess in theory if it throws an
exception it could keep retrying.
Ultimately, if we can't load the broker admin context that's _really_ bad. I
am going to look at reworking this because this really should be a fatal error.
I first tried to do this in the constructor but ran into the stack overflow
issue because creating that context requires the Broker filter chain to be
complete so we can't call that during the construction. I next tried to add a
start() method override and do it during start but that was also a problem. It
turns out that the broker [loads destinations
](https://github.com/apache/activemq/blob/f84eb3968a02bb5a9698da508d66c56c72ccdaaf/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java#L738)
and creates them which triggers the advisory callbacks _before_ [start() is
called
](https://github.com/apache/activemq/blob/f84eb3968a02bb5a9698da508d66c56c72ccdaaf/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java#L746)
so I ended up with this lazy load idea.
However I don't really like this design. This object should really be
initialized on start and re-used for the lifecycle. If we can't load it it
should fail fast as that's a fatal error. I'm going to look more into it
because I realized that while the broker may be trying to send advisories
during destination loading before start() is called, is that really doing
anything? At that point there would be no consumers for the advisories anyways
so I need to investigate this more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
