You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by hmcl <gi...@git.apache.org> on 2016/11/01 17:34:55 UTC
[GitHub] storm pull request #1757: Apache master storm 2182 top storm 1694
GitHub user hmcl opened a pull request:
https://github.com/apache/storm/pull/1757
Apache master storm 2182 top storm 1694
This patch complements addresses the code review comments in STORM-1694 concerning the examples. It was the cleanest way to address them was to create its own patch. That's the reason this patch sits on top of STORM-1694 and should be merged only after the other patch is merged.
@harshach @HeartSaVioR can you please take a look. Thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hmcl/storm-apache Apache_master_STORM-2182_top_STORM-1694
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1757.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 #1757
----
commit 76d6e7e5249fe564dee1a3b92b9d9799cb50ef4c
Author: Hugo Louro <hm...@gmail.com>
Date: 2016-06-21T16:35:16Z
STORM-1694: Kafka Spout Trident Implementation Using New Kafka Consumer API
- Kafka New Client - Opaque Transactional Trident Spout Implementation
- Implementation supporting multiple named topics and wildcard topics
commit 4d22a68abb94a0f2a4bcd5cdc4d512ae0a9a0590
Author: Hugo Louro <hm...@gmail.com>
Date: 2016-10-25T22:48:21Z
STORM-2182: Refactor Storm Kafka Examples Into Own Modules
- Created modules
- storm-kafka-examples
- storm-kafka-client-examples
----
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1757
@hmcl If it's really common so that most of examples for modules could refer it, I'd make common module for examples without dependencies for external storage. Or we could even place them to storm-core if it's also useful for end-users and it's not related to any external storage.
Like I said I'm fine to what you propose from PR, just would like to see the way to avoid dependencies coupling at any chance, which would be better in my perspective.
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1757
@HeartSaVioR why are we trying to avoid the module dependencies so much, to the point of creating duplicate code? Shouldn't we try to avoid creating duplicate code at all costs?
If we eliminate all the module dependencies I created, we will have to duplicate 4 classes. Two of them will be duplicated in 2 places each, and the other 2 duplicated in one place. We will have 6 classes more than we need with the current module structure.
---
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 #1757: STORM-2182: Refactor Storm Kafka Examples Into Own...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1757
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1757
@hmcl I agree that we should try to avoid duplicated code, but I also have seen that unneeded dependency makes dependency problem which is not directed from first level deps but transitive dependencies.
If common codes are not related to kafka API, I'd rather to have common module between the twos, but if it's not (and even if it is), either way is OK to me. That was just my preference, and others could be thinking differently, and don't have strong opinion about this.
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1757
@HeartSaVioR so that means that we can merge this in and address those improvements later?
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1757
@HeartSaVioR I am OK with a common module, but does it make sense to have a common module, with little or no dependencies, just to hold a couple of classes, and have both kafka modules refer to it? In this particular case, one of the classes that need to be reused lives in storm-starter, and three other classes are common to both storm-kafka and storm-kafka-client.
Should we create a common module to all the examples ? Or should the common module be storm-starter ?
---
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 issue #1757: STORM-2182: Refactor Storm Kafka Examples Into Own Module...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1757
@harshach changed the title of the PR.
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1757
@hmcl Yes either filing an issue or just forget and readdress someone reclaims are fine for me. Please note that I didn't take a look in detail. I'm not familiar with storm-kafka-client (even storm-kafka in detail), and other PRs are waiting a line too.
@harshach Please go ahead if you took a look at.
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1757
@hmcl already +1ed on other PR.
@HeartSaVioR @hmcl we can handle cleaning up examples in another PR.
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1757
Before looking into the detail, how much storm-kafka-example and storm-kafka-client-example shares the code? Personally I'm OK to have some duplications if it can get rid of dependency between modules.
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1757
@hmcl I wish we have common module for same patterns on example modules, which could be a new module, or even including contents to storm-core. (Only when common module doesn't have coupling to external module) But since this is example code, this is not critical thing.
Let's review more on detail first.
---
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 issue #1757: STORM-2182: Refactor Storm Kafka Examples Into Own Module...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1757
@hmcl merged into master but can't cherry-pick into storm-1.x. Here is the error I am getting
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project storm-kafka-examples: Compilation failure
[ERROR] /Users/harsha/code/apache/storm/examples/storm-kafka-examples/src/main/java/org/apache/storm/kafka/trident/KafkaProducerTopology.java:[47,41] incompatible types: org.apache.storm.kafka.bolt.mapper.FieldNameBasedTupleToKafkaMapper<java.lang.Object,java.lang.Object> cannot be converted to org.apache.storm.kafka.bolt.mapper.TupleToKafkaMapper<java.lang.String,java.lang.String>
[ERROR] -> [Help 1]
can you fix this and open a new PR for 1.x-branch
---
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 issue #1757: Apache master storm 2182 top storm 1694
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1757
@harshach @HeartSaVioR I am fine either way. @HeartSaVioR, of all the alternatives that we discussed, can you please let me know which one you prefer ? It should be an easy fix, and I can either address it now, or in a subsequent PR, where we can also take a look at all the examples and see if it's adequate to do further refactoring of the package storm-starter.
---
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.
---