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

[#754][#743] update to be compatible with storm 1.x.x #769

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mazhechao
Contributor

mazhechao commented May 20, 2016

The most change is that the package name changes from backtype to org.apache, and just fix that.

@mazhechao

This comment has been minimized.

Show comment
Hide comment
@mazhechao

mazhechao May 23, 2016

Contributor

@costin Will you have a look at it please?

Contributor

mazhechao commented May 23, 2016

@costin Will you have a look at it please?

@costin

This comment has been minimized.

Show comment
Hide comment
@costin

costin May 23, 2016

Member

Thanks @mazhechao
As I've mentioned in #754, I'm torn on whether to upgrade to Storm 1.0 or keep 0.9 around for a while. A compromise might be to upgrade master (5.0.0) to Storm 1.0 and direct Storm users of 0.9 to 2.x

Member

costin commented May 23, 2016

Thanks @mazhechao
As I've mentioned in #754, I'm torn on whether to upgrade to Storm 1.0 or keep 0.9 around for a while. A compromise might be to upgrade master (5.0.0) to Storm 1.0 and direct Storm users of 0.9 to 2.x

@mazhechao

This comment has been minimized.

Show comment
Hide comment
@mazhechao

mazhechao May 24, 2016

Contributor

I agree to upgrade master(5.0.0) to Storm 1.0, as there are many Storm users using 0.9.x now.

I notice that storm-elasticsearch has been merged into Storm project since 1.0.0. However storm-ealsticsearch doesn't support ES 2.x now. As a result, those who use storm-elasticsearch cannot use ES 2.x. If es-hadoop supports Storm 1.0, users can use both new Storm and ES at the same time.

Contributor

mazhechao commented May 24, 2016

I agree to upgrade master(5.0.0) to Storm 1.0, as there are many Storm users using 0.9.x now.

I notice that storm-elasticsearch has been merged into Storm project since 1.0.0. However storm-ealsticsearch doesn't support ES 2.x now. As a result, those who use storm-elasticsearch cannot use ES 2.x. If es-hadoop supports Storm 1.0, users can use both new Storm and ES at the same time.

costin added a commit that referenced this pull request May 25, 2016

[STORM] Make code compile with Storm 1.0.1
Use proper package for Storm's Guava classes
Find a compromise between log4j2 (used by Storm) and log4j used by the
 rest of the world including ES
Note when ran in a batch, some tests are still failing while running fine
in isolation.

relates #769

costin added a commit that referenced this pull request May 25, 2016

[SPARK] Polish tests a bit to cope with the changes in Storm
Internally Storm has changed yet again causing race conditions which are
hard to pick in the tests which fail for no apparent reason

relates #769
@costin

This comment has been minimized.

Show comment
Hide comment
@costin

costin May 25, 2016

Member

This has been merged into master (upcoming 5.0-alpha3).
Several things to note:

  1. the PR was not complete and the code did not compile (the guava classes in Storm had been moved to a different package as well).
  2. Storm 1.x also depends on log4j2 instead of log4j 1 and is not really compatible. Since ES and the rest of the libraries do depend on log4j 1 resolving this was somewhat problematic. I saw you tried to resolve the conflict in the classpath but that only worked for Storm while breaking all the other libraries. However removing the conflicting slf4j api seems to be solved the issue.

Last but not least the integration tests are failing with Storm 1.x. It's the same old problem of race conditions and concurrency - starting the topology and waiting for tuple ingestion is not easy since Storm doesn't offer any utility classes (as far as I know) and while the tests do pass in isolation, in bulk things start to trip.
I've tweaked them a bit but it doesn't seem to be enough - I've raised a separate issue for this. Going forward I'm seriously thinking of moving to unit tests since each release of Storm seems to break things a bit in this department.

@mazhechao Thank for your contribution and hopefully there will be many more in the future! Cheers!

Member

costin commented May 25, 2016

This has been merged into master (upcoming 5.0-alpha3).
Several things to note:

  1. the PR was not complete and the code did not compile (the guava classes in Storm had been moved to a different package as well).
  2. Storm 1.x also depends on log4j2 instead of log4j 1 and is not really compatible. Since ES and the rest of the libraries do depend on log4j 1 resolving this was somewhat problematic. I saw you tried to resolve the conflict in the classpath but that only worked for Storm while breaking all the other libraries. However removing the conflicting slf4j api seems to be solved the issue.

Last but not least the integration tests are failing with Storm 1.x. It's the same old problem of race conditions and concurrency - starting the topology and waiting for tuple ingestion is not easy since Storm doesn't offer any utility classes (as far as I know) and while the tests do pass in isolation, in bulk things start to trip.
I've tweaked them a bit but it doesn't seem to be enough - I've raised a separate issue for this. Going forward I'm seriously thinking of moving to unit tests since each release of Storm seems to break things a bit in this department.

@mazhechao Thank for your contribution and hopefully there will be many more in the future! Cheers!

@costin costin closed this May 25, 2016

@mazhechao

This comment has been minimized.

Show comment
Hide comment
@mazhechao

mazhechao May 25, 2016

Contributor

@costin Thanks for your review and comment.

Contributor

mazhechao commented May 25, 2016

@costin Thanks for your review and comment.

withccm added a commit to withccm/elasticsearch-hadoop that referenced this pull request Jun 21, 2016

[STORM] Make code compile with Storm 1.0.1
Use proper package for Storm's Guava classes
Find a compromise between log4j2 (used by Storm) and log4j used by the
 rest of the world including ES
Note when ran in a batch, some tests are still failing while running fine
in isolation.

relates #769

withccm added a commit to withccm/elasticsearch-hadoop that referenced this pull request Jun 21, 2016

[SPARK] Polish tests a bit to cope with the changes in Storm
Internally Storm has changed yet again causing race conditions which are
hard to pick in the tests which fail for no apparent reason

relates #769

@mazhechao mazhechao changed the title from [#754][#743] update to compat with storm 1.x.x to [#754][#743] update to be compatible with storm 1.x.x Nov 3, 2016

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