You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by arunmahadevan <gi...@git.apache.org> on 2016/01/07 10:59:20 UTC
[GitHub] storm pull request: [STORM-1499] Fix Kafka spout to maintain backw...
GitHub user arunmahadevan opened a pull request:
https://github.com/apache/storm/pull/994
[STORM-1499] Fix Kafka spout to maintain backward compatibility
STORM-1220 introduced some changes where in one place it passes ByteBuffer as is instead of byte[].
Existing bolts (0.10.0) expects a byte[] and fails with
"java.lang.RuntimeException: java.lang.ClassCastException: java.nio.HeapByteBuffer cannot be cast to [B "
This patch addresses the issue by emiting byte[] instead of ByteBuffer.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arunmahadevan/storm STORM-1499
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/994.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #994
----
commit 1eca8a3398679dce7675e72d91527ac05335bd54
Author: Arun Mahadevan <ai...@hortonworks.com>
Date: 2016-01-07T09:56:00Z
[STORM-1499] Fix Kafka spout to maintain backward compatibility
STORM-1220 introduced some changes where in one place it passes ByteBuffer as is instead of byte[].
Existing bolts (0.10.0) expects a byte[] and fails with
"java.lang.RuntimeException: java.lang.ClassCastException: java.nio.HeapByteBuffer cannot be cast to [B "
This patch addresses the issue by emiting byte[] instead of ByteBuffer.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173027654
We can either just include this as a warning as part of Release Notes or we can roll this change back. I am fine with including just a Release Note to warn all users trying to upgrade unless others thing Rolling upgradability is important enough that we only brake it when people move between major versions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173260026
@revans2 I see what you are saying. Will document this incompatibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1499] Fix Kafka spout to maintain backw...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/994
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1499] Fix Kafka spout to maintain backw...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-169832464
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1499] Fix Kafka spout to maintain backw...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-170614876
Looks like this is for https://issues.apache.org/jira/browse/STORM-1449 not 1499.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173233553
It sounds like everyone is fine with the current set of incompatibilities, so it comes down to a documentation issue. I looked at the storm-kafka docs and I don't see anywhere that this incompatability is mentioned. In fact in the section that talks about the MultiSchema it needs to be updated to reflect the changes from STORM-1220.
Could someone on this list file a JIRA or just update the documentation to explain the incompatibility and how to upgrade?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173077993
@priyank5485 @revans2 @Parth-Brahmbhatt the changes introduced by STORM-1220 has not yet gone out in any releases. It will go out in 1.0 but with the fix in this PR (which is also part of 1.0) the changes are backward compatible. i.e. the KafkaSpout would emit byte[] and not ByteBuffer as values in the tuples. So the old bolts (that expects a byte[]) would continue to work with the new KafkaSpout.
I assume @priyank5485 got the issue when the jar was built from master branch before this PR got merged in and got bundled with the client.
Correct me if I am missing something.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1499] Fix Kafka spout to maintain backw...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-170585618
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-172976476
@priyank5485 you are correct. I didn't look into this in great detail until now. What is more if you submit using an old client you will get deserialization errors too, because the SerialId is not set.
STORM-1220 seems to fundamentally not be able to maintain that backwards compatibility. So now the question is are the benefits of STORM-1220 worth the incompatibility? To me it seems likely that they are, because it is just updating a dependency.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by priyank5485 <gi...@git.apache.org>.
Github user priyank5485 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173024682
@revans2 I am fine with the changes except one major concern pointed out by @Parth-Brahmbhatt I confirmed this with him and our main concern would be the rolling upgrade feature for topologies with kafka spout. That would not work anymore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173362478
Maintaining backward compatibility is bi-directional, which means just modifying core can break module's backward compatibility what @revans2 stated.
We should bear in mind of backward compatibility with core itself.
Adopting semver is great to go, and in fact preparing 1.0.0 is the chance to break backward compatibility if necessary. If we were deprecated some classes/methods, let's review.
I expect we can not have a chance to release another major version sooner (except 2.0, it is not for such change)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-170616167
Sorry for the typo, changed the subject.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173238098
Filed JIRA and will update the doc - https://issues.apache.org/jira/browse/STORM-1486
There is no incompatibility since the emitted tuple values continue to be byte[]. Only the api signatures of the MultiScheme (and other schemes) have changed. It now accepts a ByteBuffer instead of byte[] but returns tuples with byte[] as values as before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by priyank5485 <gi...@git.apache.org>.
Github user priyank5485 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-171576791
Just ran in to an issue where this fix does not work. Correct me if i am wrong. But a fat storm jar that is compiled with older version of storm-kafka will still use byte[] and the newer version of storm now uses ByteBuffer. Hence it throws an exception at runtime if storm is upgraded. I think upgrade of storm should still work with older topology jars.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1449] Fix Kafka spout to maintain backw...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/994#issuecomment-173246546
@arunmahadevan There are several incompatibilities. Most of them don't matter because this is a part of the 1.x release, so rolling upgrades/etc will not be possible.
The one incompatibility that remains is that you cannot use a pre 1.x kafka spout on a 1.x cluster. It is always good practice to maintain the versions the same, but in this case it is required.
KafkaUtils.generateTuples, calls the modified MultScheme.deserialize API making this a problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---