Skip to content

INT-4544: Allow runtime MBeans (un)registered #2601

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

Merged
merged 10 commits into from
Oct 19, 2018

Conversation

artembilan
Copy link
Member

JIRA: https://jira.spring.io/browse/INT-4544

  • Fix SourcePollingChannelAdapterFactoryBean to register an
    outputChannelName into the SourcePollingChannelAdapter for late
    binding, especially in case of dynamic IntegrationFlow registrations.
    This must be back-ported to 5.0.x
  • Implement DestructionAwareBeanPostProcessor in the
    IntegrationMBeanExporter for the runtime beans tracking, e.g. via
    dynamic IntegrationFlow registrations
  • Refactor IntegrationMBeanExporter for static and dynamic beans
    registration and destruction
  • Remove unused properties; introduce some new for tracking beans and
    their relationship

JIRA: https://jira.spring.io/browse/INT-4544

* Fix `SourcePollingChannelAdapterFactoryBean` to register an
`outputChannelName` into the `SourcePollingChannelAdapter` for late
binding, especially in case of dynamic `IntegrationFlow` registrations.
This must be back-ported to `5.0.x`
* Implement `DestructionAwareBeanPostProcessor` in the
`IntegrationMBeanExporter` for the runtime beans tracking, e.g. via
dynamic `IntegrationFlow` registrations
* Refactor `IntegrationMBeanExporter` for static and dynamic beans
registration and destruction
* Remove unused properties; introduce some new for tracking beans and
their relationship
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all after a quick review - will perform a more thorough review tomorrow (after travis).

@@ -300,6 +306,93 @@ public void afterSingletonsInstantiated() {

}

private void registerProducer(MessageProducer messageProducer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be below the next method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is called from the afterSingletonsInstantiated() first time, so this is normal order for the private methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's also called from postProcessAfterInitialization(). I believe the rule is called methods should always be below the last place that calls it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me, but I've followed this rule so far: https://github.com/spring-projects/spring-framework/wiki/Code-Style#java-source-file-organization

Note that private or protected methods called from method implementations should be placed immediately below the methods where they're used. In other words if there 3 interface method implementations with 3 private methods (one used from each), then the order of methods should include 1 interface and 1 private method in sequence, not 3 interface and then 3 private methods at the bottom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just my interpretation then; I guess those guidelines don't specifically address this use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Fine with me.
Will fix. 😄

Not sure though, what happened to the org.springframework.integration.monitor.MessageSourceTests on Travis...

`postProcessAfterInitialization()`
* Refactor JMX test configurations to reuse MBeanServer as much as
possible and destroy the server whenever it is necessary
@garyrussell
Copy link
Contributor

Looks like there are still test issues (instance already exists exceptions).

@artembilan
Copy link
Member Author

Yeah... Looks like MBean Server is not closed somewhere yet...

Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And still travis failure.


@Override
public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
if (this.singletonsInstantiated) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are already at RC, perhaps we should add a try {...} catch {...} under here to fail soft (and log an error) instead of failing hard (just in case there are any regression issues) ?

…ialization()`

to avoid breaking changes for the current GA phase
* Do not unregister those MBeans explicitly which weren't created at
runtime
* Do not use a cst in the `StandardIntegrationFlowContext` for
`BeanFactory`, but an explicit `BeanDefinitionRegistry`
* Extract targets from proxies in the
`IntegrationMBeanExporter.postProcessAfterInitialization()`
* Remove logging in the `MBeanExporterIntegrationTests`
similar in the `foo` in the `MessageSourceTests`
* Use `MBeanServer` bean in the `Int2307Tests`
* Use JUnit 5ctor injection injection in the `MessageMetricsAdviceTests`
on the server provided by the managed `MBeanServerFactoryBean` which
destroys a server on application context close
* Fix `StoredProcJmxManagedBeanTests-context.xml` to use `MBeanServer`
from the `<context:mbean-server>`
@artembilan
Copy link
Member Author

OK! Finally it is green 😄

Sorry for the mess in tests.

Note: When we configure runtime beans, we don't process plain MessageHandler and MessageSource beans. I would say without endpoints it does not make sense to have MBeans for plain beans.
Plus there is the problem that postProcessAfterInitialization() is called for MessageHandler/MessageSource and their endpoint, but at the moment to parse one of them there is no another yet...

@garyrussell garyrussell merged commit e3ce37c into spring-projects:master Oct 19, 2018
@garyrussell
Copy link
Contributor

This must be back-ported to 5.0.x

Separate PR?

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

Successfully merging this pull request may close these issues.

2 participants