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

INT-3147 Twitter/Feed expose MessageSources as beans #910

Closed
wants to merge 3 commits into from

Conversation

ghillert
Copy link
Contributor

@ghillert ghillert commented Oct 3, 2013

  • In the Twitter Inbound Channel Adapter Parser, expose the MessageSource as top-level bean rather than as inner bean
  • In the Feed Inbound Channel Adapter Parser, expose the MessageSource as top-level bean rather than as inner bean
  • Add tests
  • Add documentation to Twitter + Feed chapter

Jira: https://jira.springsource.org/browse/INT-3147

@artembilan
Copy link
Member

This branch has to be rebased and polished

final String channelAdapterId = this.resolveId(element, sourceBuilder.getRawBeanDefinition(), parserContext);
final String sourceBeanName = channelAdapterId + ".source";

parserContext.getRegistry().registerBeanDefinition(sourceBeanName, sourceBuilder.getBeanDefinition());
Copy link
Member

Choose a reason for hiding this comment

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

Looks like should be a common case for all MessageSources with all inbound-channel-adapters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This crossed my mind. @garyrussell and I chatted about the current way JMX support is provided (MessageSourceMetrics) and he mentioned that an overhaul would be nice, preferring a more direct approach. Although this would admittedly be a lot of work.

I am certainly not keen having to make this explicit call to resolveId(). Would be nice if that was already available somehow - I did not want to break anything though :-)

@artembilan
Copy link
Member

Regarding metadataKey issues.
How about to make id attribute of these adapters as required and use it as a key for MetadataStore? Similar to <delayer>, where its id is used as groupId.
In this case we solve all issues:

  • uniqueness
  • size (in case of JDBC)
  • and simiral key will determine unambiguously the source for metadata.

WDYT?

* In the Twitter Inbound Channel Adapter Parser, expose the MessageSource as top-level bean rather than as inner bean
* Add tests

Jira: https://jira.springsource.org/browse/INT-3147
* In the Feed Inbound Channel Adapter Parser, expose the MessageSource as top-level bean rather than as inner bean
* Add test

Jira: https://jira.springsource.org/browse/INT-3147
* Add documentation to Twitter chapter
* Add documentation to Feed chapter
@ghillert
Copy link
Contributor Author

ghillert commented Oct 4, 2013

Rebased.

@ghillert
Copy link
Contributor Author

ghillert commented Oct 4, 2013

@artembilan I think I would rather go down the route of exposing a property metadata-key, so users can define their own and if they don't define it, the logic falls back to the one above. But that would be a new Jira.

This PR simply addresses the bug (very unlikely the original implementor assumed that (inner bean) is used for the key rather than the component id). I did not modify any existing logic. That being said, the key does change - therefore need to make a mental note, to add this to the migration guide once merged.

@artembilan
Copy link
Member

OK.
@garyrussell, WDYT?
Thanks

@ghillert
Copy link
Contributor Author

ghillert commented Oct 7, 2013

Any updates on this? @garyrussell @artembilan

Started working on

https://jira.springsource.org/browse/INT-1941
https://jira.springsource.org/browse/INT-3167

Need this PR merged in order to start writing some JMX tests.

My branch:

https://github.com/ghillert/spring-integration/tree/INT-1941

@garyrussell
Copy link
Contributor

I am a bit tied up right now; will try to look this afternoon.

@artembilan
Copy link
Member

Hi there!
So, my opinion is:

  • register all .source beans, as we do it for .handler
  • allow configure several MetadataStore beans and provide ref attribute to adapters. Similar to MessageStore
  • do not provide metadata-key, but just rely on required id attribute
  • add MetadataStore.remove(String key)

Of course, it may be out of this PR.
Gunnar, sorry, I don't understand your position: we are in front of the major release: why don't break a bit existing non-stable logic and make it more robust, flexible and useful.
Thanks

@garyrussell
Copy link
Contributor

My 2c for this PR...

  1. Move the .source registration to the abstract class
  2. Make the ID required on the twitter adapter (agree with Artem - we should be consistent with other components, such as the delayer)

And...

  1. Create a JIRA to support explicit MDS declarations instead of (or in addition to) a global one.
  2. Create a JIRA for metadata.remove() (exposed as @ManagedOperation)
  3. Create a JIRA to consider how to get one MBean instead of two when an embedded component (handler, source) is a @ManagedResource.

@garyrussell
Copy link
Contributor

Hi @ghillert are you going to wrap this up, or do you want one of us to?

Thanks.

@ghillert
Copy link
Contributor Author

@garyrussell Will try later today and over the weekend.

@artembilan
Copy link
Member

Superseded by #928

@artembilan artembilan closed this Oct 17, 2013
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.

None yet

3 participants