You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by vesense <gi...@git.apache.org> on 2018/01/18 14:27:48 UTC
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
GitHub user vesense opened a pull request:
https://github.com/apache/storm/pull/2518
STORM-2902: Some improvements for storm-rocketmq module
## Brief changelog
* Upgraded RocketMQ version to 4.2.0 which brings improvements and new features like batch sending
* Imporved retry policy for RocketMQ consumer push mode to avoid data loss in some scenes
* Batch sending supported for bolt and trident state
* Allow running several consumer instances in one process, that is to say, different topics in one worker is possible
## Verifying this change
local tests check passed ✅
apache-rat check passed ✅
checkstyle check passed ✅
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vesense/storm rocketmq-improvements
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2518.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 #2518
----
commit d3232136655647f1968b2b8b0ba22f84f7d1f96c
Author: Xin Wang <xi...@...>
Date: 2017-12-24T13:27:40Z
STORM-2902: Some improvements for storm-rocketmq module
----
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2518
Any comments are welcome..
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2518
@vongosling Added unit tests and rebased PR.
---
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
Posted by hustfxj <gi...@git.apache.org>.
Github user hustfxj commented on a diff in the pull request:
https://github.com/apache/storm/pull/2518#discussion_r179943857
--- Diff: external/storm-rocketmq/src/main/java/org/apache/storm/rocketmq/spout/RocketMqSpout.java ---
@@ -60,14 +58,14 @@
public class RocketMqSpout implements IRichSpout {
// TODO add metrics
--- End diff --
not only add metrcis, but also print log for MqSpout and MqBolt
---
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2518
---
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on a diff in the pull request:
https://github.com/apache/storm/pull/2518#discussion_r166494277
--- Diff: external/storm-rocketmq/src/main/java/org/apache/storm/rocketmq/RocketMqConfig.java ---
@@ -23,28 +23,20 @@
import java.util.Properties;
import java.util.UUID;
+import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.Validate;
import org.apache.rocketmq.client.ClientConfig;
import org.apache.rocketmq.client.consumer.DefaultMQPushConsumer;
import org.apache.rocketmq.client.exception.MQClientException;
import org.apache.rocketmq.client.producer.DefaultMQProducer;
import org.apache.rocketmq.common.consumer.ConsumeFromWhere;
-import org.apache.rocketmq.remoting.common.RemotingUtil;
/**
* RocketMqConfig for Consumer/Producer.
*/
--- End diff --
RocketMQ
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2518
@hustfxj Added logs and fixed warnings for RocketMqSpout/RocketMqBolt.
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/storm/pull/2518
Good Catch, what's the plan for this pr? I noticed that we have kept the unmerged status for a long time.
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/storm/pull/2518
glad to see the update of this pr :-)
---
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on a diff in the pull request:
https://github.com/apache/storm/pull/2518#discussion_r180281409
--- Diff: external/storm-rocketmq/src/main/java/org/apache/storm/rocketmq/spout/RocketMqSpout.java ---
@@ -60,14 +58,14 @@
public class RocketMqSpout implements IRichSpout {
// TODO add metrics
--- End diff --
Thanks. Will update.
---
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on a diff in the pull request:
https://github.com/apache/storm/pull/2518#discussion_r178704108
--- Diff: external/storm-rocketmq/src/main/java/org/apache/storm/rocketmq/RocketMqConfig.java ---
@@ -23,28 +23,20 @@
import java.util.Properties;
import java.util.UUID;
+import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.Validate;
import org.apache.rocketmq.client.ClientConfig;
import org.apache.rocketmq.client.consumer.DefaultMQPushConsumer;
import org.apache.rocketmq.client.exception.MQClientException;
import org.apache.rocketmq.client.producer.DefaultMQProducer;
import org.apache.rocketmq.common.consumer.ConsumeFromWhere;
-import org.apache.rocketmq.remoting.common.RemotingUtil;
/**
* RocketMqConfig for Consumer/Producer.
*/
--- End diff --
@vongosling This is limited by the storm checkstyle rule `AbbreviationAsWordInName`. Refer to https://github.com/apache/storm/blob/master/storm-checkstyle/src/main/resources/storm/storm_checkstyle.xml#L213
---
[GitHub] storm pull request #2518: STORM-2902: Some improvements for storm-rocketmq m...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on a diff in the pull request:
https://github.com/apache/storm/pull/2518#discussion_r166494139
--- Diff: external/storm-rocketmq/README.md ---
@@ -1,6 +1,6 @@
# Storm RocketMQ
-Storm/Trident integration for [RocketMQ](https://rocketmq.incubator.apache.org/). This package includes the core spout, bolt and trident states that allows a storm topology to either write storm tuples into a topic or read from topics in a storm topology.
+Storm/Trident integration for [RocketMQ](https://rocketmq.apache.org/). This package includes the core spout, bolt and trident states that allows a storm topology to either write storm tuples into a topic or read from topics in a storm topology.
--- End diff --
great！
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2518
@vongosling
I'm aware of RocketMQ, just meant that we have unmanaged modules nowadays in Storm repo. since many of us are focusing core, and some of us are making Kafka connector possible to be maintained as first class support.
IMHO, I don't really want to pay maintaining cost to unmanaged modules. I don't think connectors are served as first class support which have less than 2 `active` committer sponsors or at least 1 `active` committer sponsor and 1 `active` contributor (in this case committer sponsor couldn't craft pull requests as he/she may not get binding +1 like this case), and they are better to be maintained outside of Storm core. But the opinion is on my own, and some other committers/PMCs might take a look at.
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2518
+1
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2518
Also rebased PR.
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/storm/pull/2518
@HeartSaVioR RocketMQ is another high performance messaging and low latency engine in Apache, I am glad to introduce it to your storm guys :-)
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2518
@harshach @vongosling @hustfxj Do yo have time to take a look?
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by hustfxj <gi...@git.apache.org>.
Github user hustfxj commented on the issue:
https://github.com/apache/storm/pull/2518
+1 Thank you for @vesense
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2518
I'm sad to say this is the case, same case as storm-eventhub which lacks committer sponsors. I can't (and I shouldn't) review connector which I have completely no idea what it is and how it works and how the API looks like. I feel at least @hustfxj needs to be another committer sponsor and maintain actively making a pair, but if it doesn't occur, I'm not sure we can maintain this connector in Storm repo. I have same feeling for storm-eventhub, and maybe some others as well.
---
[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2518
@vongosling Will add more unit tests later.
---