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

Xd 3244 Initial port of XD MessageBus #8

Closed
wants to merge 7 commits into from

Conversation

dturanski
Copy link
Contributor

This brings over XD MessageBus code basically as-is:

  • Some job related functionality that depends on JobPlugin, etc. was deleted along with corresponding tests
  • I may have another commit on this if not already merged (see below) , but I don't recommend waiting.
  • KafkaMessageBusTests is failing trying to connect to ZK. (@ignored) @mbogoevici can have a look, but kafka is not a high priority.

The test code had to be rearranged to bring in relevant MB test packages from DIRT and some of the Test harness needed refactoring to sort out dependencies. Currently everything is under spring-cloud-streams-bindings. There is a common test project spring-cloud-streams-binding-test. This should take another iteration in a future PR to separate out transport specific things as much as possible. spring-xd-dirt supported testing all transports and had some common base classes, etc. these were dealt out to the binding-* components to get to this point.

@mbogoevici
Copy link
Contributor

I wouldn't block this b/c of Kafka.

*
* @author Marius Bogoevici
*/
public class WindowingOffsetManager implements OffsetManager, InitializingBean, DisposableBean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class is shared with the Kafka Source so we should ideally move it to Spring Integration Kafka eventually. Beyond the scope of this PR, but needs a reminder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a TODO

@garyrussell
Copy link
Contributor

There's something funky with this PR and STS; looks like there's a transitive dep on xd-codec someplace - I am seeing unused imports (kryo.*, MutiTypeCodec) in ChannelBindingAdapterConfiguration.

When I fix imports (in STS) it pulls in 2 imports for each). In the CP, spring-xd-codec is there.

protected static class CodecConfiguration {
@Autowired
ApplicationContext applicationContext;

@Bean
@ConditionalOnMissingBean(name = "codec")
public MultiTypeCodec<?> codec() {
Map<String, KryoRegistrar> kryoRegistrarMap = this.applicationContext.getBeansOfType(KryoRegistrar
public MultiTypeCodec codec() {
Copy link
Contributor

Choose a reason for hiding this comment

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

raw type, needs <?>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, its from the test dep on tuple. I will exclude

ultrafox:/s2build/spring-cloud-streams/spring-cloud-streams-bindings[XD-3244]
$ mvn dependency:tree | grep spring-xd
[INFO] +- org.springframework.xd:spring-xd-tuple:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] | +- org.springframework.xd:spring-xd-codec:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] | | - org.springframework.xd:spring-xd-spi-common:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] +- org.springframework.xd:spring-xd-tuple:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] | +- org.springframework.xd:spring-xd-codec:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] | | - org.springframework.xd:spring-xd-spi-common:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] +- org.springframework.xd:spring-xd-tuple:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] | +- org.springframework.xd:spring-xd-codec:jar:1.2.1.BUILD-SNAPSHOT:test
[INFO] | | - org.springframework.xd:spring-xd-spi-common:jar:1.2.1.BUILD-SNAPSHOT:test
ultrafox:
/s2build/spring-cloud-streams/spring-cloud-streams-bindings[XD-3244]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be better now

@garyrussell
Copy link
Contributor

@dturanski re #9 you can just add this commit to your PR if you like...

garyrussell@d41745f

@garyrussell
Copy link
Contributor

Maybe it's not such a great idea to use the same packages. It can cause all kinds of problems if people have cr*p on their classpath.

@garyrussell
Copy link
Contributor

Here's an updated commit for #9

garyrussell@e653d3a

@garyrussell
Copy link
Contributor

rebased my #9 commit

garyrussell@dd01730

@markpollack markpollack self-assigned this Jul 13, 2015
@markpollack
Copy link
Contributor

Added commits for changing to JDK7 build and move transport specific packages in binding-test to specific transport test projects.

Squashed and merged as 5377eaf

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.

4 participants