Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

XD-1368 Refactor container to remove shared module context #622

Closed
wants to merge 10 commits into from

5 participants

@dturanski
Collaborator

Added a small bootstrap context that pre instantiates SharedContextInitializers, ApplicationListeners specifically for providing bean definitions (e.g., MessageBus) to the ContainerContext. The ContainerContext holds what it did before (Control channels, ModuleDeployer, Plugins) and is now also used to create shared channels and other resources created during module deployment. The intention is to simplify the XD container architecture and the ability for end users and XD developers to extend it.

This PR addresses https://jira.spring.io/browse/XD-1368 and https://jira.spring.io/browse/XD-1262 and is prerequisite to :
https://jira.spring.io/browse/XD-432
https://jira.spring.io/browse/XD-912
https://jira.spring.io/browse/XD-1342
https://jira.spring.io/browse/XD-1343
https://jira.spring.io/browse/XD-1348

.../xd/dirt/container/initializer/CommonInitializer.java
((17 lines not shown))
+package org.springframework.xd.dirt.container.initializer;
+
+import org.springframework.xd.dirt.util.ConfigLocations;
+
+
+/**
+ *
+ * @author David Turanski
+ */
+public class CommonInitializer extends AbstractXMLBeanDefinitionProvider {
+
+ private static final String CONTEXT_CONFIG_ROOT = ConfigLocations.XD_CONFIG_ROOT + "initializers/common/";
+
+ @Override
+ public int getOrder() {
+ return Integer.MIN_VALUE - 1;
@ilayaperumalg Collaborator

As I see ((Integer.MIN_VALUE - 1) == Integer.MAX_VALUE), Did you intend to make the XML files defined under common to get the lowest precedence(and thereby not be overridden by any of other initializers) of all the SharedContextInitializers?

Also, using the variables Ordered.HIGHEST_PRECEDENCE(Integer.MIN_VALUE) and Ordered.LOWEST_PRECEDENCE(Integer.MAX_VALUE) would make it easier to understand at first glance.

@dturanski Collaborator

I'm surprised that even compiles! Should be Integer.MIN_VALUE + 1 . Or in this case HIGHEST_PRECEDENCE is appropriate. I'm not sure HIGHEST_PRECEDENCE + 1 is that easy to understand though.

@ericbottard Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...amework/xd/dirt/server/ContainerBootstrapContext.java
((52 lines not shown))
+ .headless(true)
+ .web(false)
+ .run();
+ Collection<SharedContextInitializer> sharedContextInitializerBeans = bootstrapContext.getBeansOfType(
+ SharedContextInitializer.class).values();
+
+ this.sharedContextInitializers = sharedContextInitializerBeans.toArray(new
+ SharedContextInitializer[sharedContextInitializerBeans.size()]);
+ Arrays.sort(sharedContextInitializers, new OrderComparator());
+ }
+
+ SharedContextInitializer[] sharedContextInitializers() {
+ return this.sharedContextInitializers;
+ }
+
+ CommandLinePropertySourceOverridingListener<?> commandLineListener() {
@dturanski Collaborator

I'm not crazy about this commandLineListener() method. The refactoring was convenient because this class needs the listener and can create it from the options object. I think the listener is needed in every context XD creates (@ericbottard ?). The alternative is to create it externally and pass it as an arg, but then that code is duplicated in SingleNodeApplication.

@ericbottard Collaborator

It should only be needed in the "main" contexts (definitely not in the module contexts, for example, if that's what you're asking)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...integration/x/bus/MessageBusAwareChannelResolver.java
((6 lines not shown))
private final Map<String, MessageChannel> channels = new HashMap<String, MessageChannel>();
- private volatile MessageBus messageBus;
+ private final MessageBus messageBus;
+
+ public MessageBusAwareChannelResolver(MessageBus messageBus) {
+ this.messageBus = messageBus;
+ }
@Override
public void setBeanFactory(BeanFactory beanFactory) {
@markfisher Owner

looks like we don't need to override this method anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ingframework/xd/dirt/plugins/stream/StreamPlugin.java
@@ -53,13 +64,23 @@ public void preProcessModule(Module module) {
properties.setProperty("xd.stream.name", md.getGroup());
properties.setProperty("xd.module.index", String.valueOf(md.getIndex()));
module.addProperties(properties);
+ module.addListener(new ApplicationListener<ApplicationPreparedEvent>() {
+
+ @Override
+ public void onApplicationEvent(ApplicationPreparedEvent event) {
+ MessageBusAwareRouterBeanPostProcessor bpp = new MessageBusAwareRouterBeanPostProcessor(messageBus);
+ bpp.setBeanFactory(event.getApplicationContext());
+ event.getApplicationContext().getBeanFactory().registerSingleton(
@markfisher Owner

This will provide the awareness of named channels in the MessageBus to any router within a Spring XD Module. Before, we only provided that selectively (to the "router" sink module). In that case, it was known that the router was at the end of the stream, whereas now any router could return a named channel, even mid-Stream or mid-flow-within-a-Module. I'm only raising this issue so we know for sure we want that "feature".

@dturanski Collaborator

This sounds like an interesting feature, allowing arbitrary branching to another stream. I think I'm ok with it but would benefit from further discussion before we officially support it.

@markfisher Owner

Well, I'm pretty sure it is implicitly supported by this change above (adding that MessageBus-aware ChannelResolver to any router defined in any module).

@markfisher Owner

One (potentially bad) idea would be to restrict adding that BPP to sink modules that have "router" in the name. That way it's a convention, and it's assumed that you are only adding the router in the sink position - so that sending to a named channel is effectively the end of a stream.

@dturanski Collaborator

We could at least check if module type is sink. Then only sinks containing routers would be impacted. The edge case would be if a router unintentionally returned a named channel...

@markfisher Owner

Seems like a good compromise for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...ingframework/xd/dirt/plugins/stream/StreamPlugin.java
((15 lines not shown))
}
@Override
public void postProcessModule(Module module) {
- MessageBus bus = findMessageBus(module);
- bindConsumer(module, bus);
- bindProducers(module, bus);
+ bindConsumer(module, this.messageBus);
@markfisher Owner

Do these 2 lines need to be surrounded with if(messageBus != null) like the others in this class?

@ericbottard Collaborator

Or do the others really need that null check?

@markfisher Owner

The others do not need the null check IF an Assert.notNull(..) is added to the constructor. That would be the best option IMO. Thanks Eric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...mework/xd/dirt/server/ContainerServerApplication.java
((16 lines not shown))
public static void main(String[] args) {
new ContainerServerApplication().run(args);
}
public ConfigurableApplicationContext getContext() {
- return this.context;
+ return (ConfigurableApplicationContext) this.coreContext.getParent();
@markfisher Owner

This should probably protect from NPE (since coreContext is null until run() is called).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...mework/xd/dirt/server/ContainerServerApplication.java
((6 lines not shown))
public ContainerServerApplication() {
this.containerMetadata = new ContainerMetadata();
}
+ @Bean
+ public ContainerMetadata containerMetadata() {
+ return this.containerMetadata;
+ }
+
public static void main(String[] args) {
new ContainerServerApplication().run(args);
}
public ConfigurableApplicationContext getContext() {
@markfisher Owner

Should this method be named getParentContext? (seems confusing having getCoreContext but just getContext here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...mework/xd/dirt/server/ContainerServerApplication.java
((44 lines not shown))
.child(ContainerServerApplication.class)
- .listeners(commandLineListener)
+ .listeners(bootstrapContext.commandLineListener())
@markfisher Owner

Why not pass these 2 in as varargs to a single listeners(..) here?

(edit): Oh, I see the 2nd call is an array already. This seems a bit dangerous. What if boot changes so that it no longer adds but resets with subsequent calls? (I don't believe there is any javadoc on the method, so it's a bit unknown) cc @philwebb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ngframework/xd/dirt/server/SingleNodeApplication.java
((28 lines not shown))
.web(false);
container.run(args);
adminContext = admin.context();
- containerContext = container.context();
+
+ coreContext = container.context();
@markfisher Owner

I'm getting a little confused regarding "coreContext" vs. "containerContext". I wonder if core should be renamed "deployerContext" since that's primarily what lives there, the ModuleDeployer. Or alternatively maybe the containerContext is actually the one that should be renamed... it's sort of the "extensions" context, right? (plugins, initializers, etc)

...or both renamed, so the responsibility of each is the focus; something like "pluginContext" and "deployerContext". Not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...ngframework/xd/dirt/server/SingleNodeApplication.java
((11 lines not shown))
- MessageChannel containerControlChannel = containerContext.getBean(
+ MessageChannel containerControlChannel = coreContext.getBean(
@markfisher Owner

This is an example where it gets confusing "containerControlChannel" but it comes from "coreContext" and not "containerContext".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
.../main/resources/META-INF/spring-xd/plugins/common.xml
@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">
@markfisher Owner

A newline would be good here to highlight the <bean> (at first glance, I thought this was an empty config file).

@markfisher Owner

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...es/META-INF/spring-xd/transports/rabbit-container.xml
@@ -15,7 +15,7 @@
<rabbit:admin connection-factory="rabbitConnectionFactory"/>
- <rabbit:template id="rabbitTemplate" connection-factory="rabbitConnectionFactory"/>
+ <!-- <rabbit:template id="rabbitTemplate" connection-factory="rabbitConnectionFactory"/> -->
@markfisher Owner

If this template isn't needed, go ahead and delete it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tekul

Builds/tests Ok for me (with module batch jars removed - other issue). Script tests all run fine too.

@markfisher markfisher self-assigned this
@ilayaperumalg
Collaborator

Not sure I am the only one seeing this; but want to share what I see after starting the container.
It looks like the embeddedServletContainer for ContainerServerApplication's ContainerConfiguration is never set. I think we may need to enable EmbeddedServletContainerAutoConfiguration for "ContainerConfiguration"?

org.springframework.context.ApplicationContextException: Unable to start embedded container; nested exception is org.springframework.context.ApplicationContextException: Unable to start EmbeddedWebApplicationContext due to missing EmbeddedServletContainerFactory bean.
at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.onRefresh(EmbeddedWebApplicationContext.java:135)
at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:476)
at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.refresh(EmbeddedWebApplicationContext.java:120)
at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:616)
at org.springframework.boot.SpringApplication.run(SpringApplication.java:306)
at org.springframework.boot.builder.SpringApplicationBuilder.run(SpringApplicationBuilder.java:130)
at org.springframework.xd.dirt.server.ContainerServerApplication.run(ContainerServerApplication.java:115)
at org.springframework.xd.dirt.server.ContainerServerApplication.main(ContainerServerApplication.java:81)
Caused by: org.springframework.context.ApplicationContextException: Unable to start EmbeddedWebApplicationContext due to missing EmbeddedServletContainerFactory bean.
at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.getEmbeddedServletContainerFactory(EmbeddedWebApplicationContext.java:185)
at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.createEmbeddedServletContainer(EmbeddedWebApplicationContext.java:158)
at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.onRefresh(EmbeddedWebApplicationContext.java:132)

@markfisher markfisher commented on the diff
...ngframework/xd/dirt/server/SingleNodeApplication.java
((8 lines not shown))
public ConfigurableApplicationContext containerContext() {
return containerContext;
}
private void setUpControlChannels(ApplicationContext adminContext,
- ApplicationContext containerContext) {
+ ApplicationContext coreContext) {
@markfisher Owner

I think this one still needs to change, actually back to "containerContext" right?

@markfisher Owner

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...sources/META-INF/spring-xd/initializers/bus/codec.xml
@@ -3,6 +3,8 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">
+<!-- These are contributed by means of a SharedContextInitializer -->
@markfisher Owner

should update the name here to avoid confusion: OrderedContextInitializer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...s/META-INF/spring-xd/initializers/bus/message-bus.xml
@@ -3,6 +3,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">
+<!-- These are contributed by means of a SharedContextInitializer -->
@markfisher Owner

should update the name here to avoid confusion: OrderedContextInitializer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...amework/xd/dirt/server/ContainerBootstrapContext.java
((19 lines not shown))
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration;
+import org.springframework.boot.builder.SpringApplicationBuilder;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationListener;
+import org.springframework.core.OrderComparator;
+import org.springframework.xd.dirt.container.initializer.OrderedContextInitializer;
+import org.springframework.xd.dirt.server.options.CommandLinePropertySourceOverridingListener;
+import org.springframework.xd.dirt.server.options.CommonOptions;
+
+
+/**
+ * Package private class to bootstrap the Container process. Configures and instantiates
+ * {@link SharedContextInitializers} and provides them to create main container context.
@markfisher Owner

this link should be changed to OrderedContextInitializers

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...irt/config/AbstractSingleNodeInitializationTests.java
((6 lines not shown))
- protected AbstractApplicationContext moduleContext;
+ protected AbstractApplicationContext coreContext;
@markfisher Owner

these need to be renamed (container -> plugin, core -> container)

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...config/LocalControlRabbitDataInitializationTests.java
@@ -32,6 +32,6 @@ protected String getControlTransport() {
@Override
protected MessageChannel getControlChannel() {
- return context.getBean("containerControlChannel", MessageChannel.class);
+ return coreContext.getBean("containerControlChannel", MessageChannel.class);
@markfisher Owner

needs to be renamed to 'containerContext'

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
.../config/LocalControlRedisDataInitializationTests.java
@@ -32,6 +32,6 @@ protected String getControlTransport() {
@Override
protected MessageChannel getControlChannel() {
- return context.getBean("containerControlChannel", MessageChannel.class);
+ return coreContext.getBean("containerControlChannel", MessageChannel.class);
@markfisher Owner

needs to be renamed to 'containerContext'

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...d/dirt/config/LocalSingleNodeInitializationTests.java
@@ -28,6 +38,19 @@
*/
public class LocalSingleNodeInitializationTests extends AbstractSingleNodeInitializationTests {
+ @Test
+ public final void verifyContextConfiguration() {
+
@markfisher Owner

more renames required...

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
.../dirt/config/RabbitSingleNodeInitializationTests.java
@@ -53,7 +53,7 @@ protected String getTransport() {
@Override
protected MessageChannel getControlChannel() {
- AmqpInboundChannelAdapter aica = context.getBean(AmqpInboundChannelAdapter.class);
+ AmqpInboundChannelAdapter aica = coreContext.getBean(AmqpInboundChannelAdapter.class);
@markfisher Owner

core -> container

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...d/dirt/config/RedisSingleNodeInitializationTests.java
@@ -49,11 +49,11 @@ protected String getTransport() {
@Override
protected MessageChannel getControlChannel() {
- RedisQueueMessageDrivenEndpoint rqmde = context.getBean("redisInboundAdapter",
@markfisher Owner

more renames...

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff
...framework/xd/dirt/config/TestMessageBusInjection.java
((5 lines not shown))
*/
public class TestMessageBusInjection {
private static final String STREAM_PLUGIN_BEAN_ID = "streamPlugin";
public static void injectMessageBus(SingleNodeApplication application, AbstractTestMessageBus testMessageBus) {
- ConfigurableApplicationContext containerContext = application.containerContext();
- StreamPlugin streamPlugin = containerContext.getBean(StreamPlugin.class);
- ModuleDeployer moduleDeployer = containerContext.getBean(ModuleDeployer.class);
- RootBeanDefinition bDefinition = new RootBeanDefinition(TestStreamPlugin.class);
+ ConfigurableApplicationContext containerContext = application.pluginContext();
@markfisher Owner

more renames...

fixing on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher
Owner

I'll be adding one commit (to address the 'fixing on merge' comments I added) and merging this, assuming we can discover why the RabbitMQ exchanges/queues are not being cleaned up when running the build.

I do have one last comment, but I think it could be handled in a subsequent commit/PR:

Where we pass in ContainerServerApplication.class to the builders (in ContainerServerApplication as well as SingleNodeApplication), we might want to make an explicit PluginConfiguration class instead, just like there is a child ContainerConfiguration class specifically for the beans in that context.

@markfisher
Owner

rebased, squashed, polished, merged, and pushed

@markfisher markfisher closed this
@dturanski dturanski deleted the dturanski:XD-1368 branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.