[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1808 Thanks for the great work and patience. +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

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1808 @revans2 this is perfect. These 4 commits make it much easier to track down. I confused the way GitHub truncates the messages - my bad. +1 --- If your project is set up for it, you can reply

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 @hmcl github like to truncate long messages. ``` $ git log STORM-2225 commit c9f9348e3de0917020a8c903f4c9d026b7dc6204 Author: Robert (Bobby) Evans Date:

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1808 Thanks @revans2. Can you please just amend the commit log [messages](https://github.com/apache/storm/pull/1808/commits) as I believe they do not really reflect the changes in this patch. One of the

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 Everyone, Sorry this took so long. I kept getting pulled off on critical issues. I have rebased/squashed everything (except the code revert) and also fixed the issue with tabs vs spaces.

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1808 +1 My only suggestion at this point is that we file an umbrella JIRA covering some of the follow up comments that haven't been addressed, such as minor refactoring, improved (java) docs,

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1808 +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

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user srdo commented on the issue: https://github.com/apache/storm/pull/1808 There are still a few files with tabs mixed in to spaces, so autoformatting all the files might be good. I also still don't really understand why the KafkaSpoutConfig builder needs to support setting a

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 The travis test failures are unrelated. --- 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

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 @hmcl I think I addressed all of your review comments. I also re-based to deal with the merge conflict. If everything looks good I will squash some of the commits. There are a lot. --- If your

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 The test failures are unrelated --- 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

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 @srdo @harshach sorry to do this to you, but I just fixed the conflicts with STORM-2236. Sadly the fastest way I could do it was to revert the original code and implement similar functionality,

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 The test failures are unrelated and are around the integration tests that always seem to fail lately. --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 All outstanding review comments should be done now. This and the 1.x port at #1868 should be ready for a final pass and hopefully being merged in. --- If your project is set up for it, you can

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 I think I have addressed all of the review comments so far. I will try to get my 1.x version of the patch up shortly. --- If your project is set up for it, you can reply to this email and have

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 @srdo as part of backporting this to 1.x I am going to need to make a change to not use Function directly, because it is only in java 8. So to maintain compatibility between 1.x and 2.x I am going

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1808 @revans2 I would like to take a look at this before this gets merged in. --- 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

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user srdo commented on the issue: https://github.com/apache/storm/pull/1808 I think my only remaining nit is the spelling error in KafkaBolt. Thanks @revans2. I am +1 on this. --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 @srdo I think I addressed all of your review comments. --- 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

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user srdo commented on the issue: https://github.com/apache/storm/pull/1808 Sure, that makes sense. Maybe storm-kafka-client should have been marked unstable since it's fairly new, it's still in a phase where the kinks are being worked out, even if the version number says

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 @srdo for me backwards compatibility for 1.x is more a question of violating out versioning than anything else. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user srdo commented on the issue: https://github.com/apache/storm/pull/1808 I like the new configuration design. Backward compatibility is not an issue for us. I'm wondering if enough people have even switched to the new spout yet to make backward compatibility for 1.x a must.

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user d2r commented on the issue: https://github.com/apache/storm/pull/1808 I see some duplicate code and documentation. Is the idea to move from /external/storm-kafka to /external/storm-kafka-client? --- If your project is set up for it, you can reply to this email and have

[GitHub] storm issue #1808: STORM-2225: change spout config to be simpler.

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1808 I updated the docs and the examples. This is a non-backwards compatible change from the 1.x release. I have some code (not included here) that can maintain most compatibility if we really want to,