You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by yhartanto <gi...@git.apache.org> on 2016/01/28 21:40:39 UTC
[GitHub] storm pull request: Force to be compatible with heron and add test...
GitHub user yhartanto opened a pull request:
https://github.com/apache/storm/pull/1056
Force to be compatible with heron and add test topology.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yhartanto/storm experiment
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1056.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 #1056
----
commit 9ccba400766b2691cabc3ef8dab6db4c79e69247
Author: Yohan Hartanto <yh...@twitter.com>
Date: 2016-01-04T04:53:19Z
Force to be compatible with heron and add test topology.
----
---
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: Force to be compatible with heron and add test...
Posted by dossett <gi...@git.apache.org>.
Github user dossett commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-186407865
Thanks! Good learning.
---
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: Force to be compatible with heron and add test...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-176860036
Can you please create a JIRA and assign a JIRA number in the PR title
---
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: Force to be compatible with heron and add test...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1056#discussion_r51264908
--- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaSpout.java ---
@@ -78,9 +82,11 @@ public void open(Map conf, final TopologyContext context, final SpoutOutputColle
if (zkPort == null) {
zkPort = ((Number) conf.get(Config.STORM_ZOOKEEPER_PORT)).intValue();
}
+ if (zkServers == null) throw new IllegalStateException("zkServers is null");
stateConf.put(Config.TRANSACTIONAL_ZOOKEEPER_SERVERS, zkServers);
+ if (zkPort == null) throw new IllegalStateException("zkPort is null");
stateConf.put(Config.TRANSACTIONAL_ZOOKEEPER_PORT, zkPort);
- stateConf.put(Config.TRANSACTIONAL_ZOOKEEPER_ROOT, _spoutConfig.zkRoot);
--- End diff --
Why do we want to drop the transactional root?
---
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: Force to be compatible with heron and add test...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-176704157
@yhartanto can you please a file jira on what this PR intendeds to solve.
---
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: Force to be compatible with heron and add test...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-176521472
I'm sorry, but I can't totally understand what the pull request is.
Apache Storm doesn't need to be compatible with Heron which is closed-source.
@yhartanto Could you clarify?
---
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: Force to be compatible with heron and add test...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-178288859
There's been no response from the submitter, and there's not a clear definition of what this aims to address.
I'm +1 for closing 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 pull request: Force to be compatible with heron and add test...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1056
---
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: Force to be compatible with heron and add test...
Posted by dossett <gi...@git.apache.org>.
Github user dossett commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-186397905
This appears to have been merged to master, was that intentional?
---
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: Force to be compatible with heron and add test...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-186407343
@dossett It wasn't merged. Just a special commit to close 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 pull request: Force to be compatible with heron and add test...
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-176525935
+1 for the number of changes in this PR I would hope there would be some more description of the *what* and the *why*.
---
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: Force to be compatible with heron and add test...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1056#issuecomment-182602247
@ptgoetz, Agreed.
-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: Force to be compatible with heron and add test...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1056#discussion_r51265457
--- Diff: external/storm-kafka/src/jvm/storm/kafka/ZkState.java ---
@@ -33,20 +32,29 @@
import java.util.Map;
public class ZkState {
+
+ public static final String TRANSACTIONAL_ZOOKEEPER_SERVERS = "transactional.zookeeper.servers";
+ public static final String TRANSACTIONAL_ZOOKEEPER_PORT = "transactional.zookeeper.port";
+ public static final String STORM_ZOOKEEPER_SESSION_TIMEOUT = "storm.zookeeper.session.timeout";
+ public static final String STORM_ZOOKEEPER_CONNECTION_TIMEOUT = "storm.zookeeper.connection.timeout";
+ public static final String STORM_ZOOKEEPER_RETRY_TIMES = "storm.zookeeper.retry.times";
+ public static final String STORM_ZOOKEEPER_RETRY_INTERVAL = "storm.zookeeper.retry.interval";
+
+
public static final Logger LOG = LoggerFactory.getLogger(ZkState.class);
CuratorFramework _curator;
private CuratorFramework newCurator(Map stateConf) throws Exception {
- Integer port = (Integer) stateConf.get(Config.TRANSACTIONAL_ZOOKEEPER_PORT);
+ Integer port = (Integer) stateConf.get(TRANSACTIONAL_ZOOKEEPER_PORT);
String serverPorts = "";
- for (String server : (List<String>) stateConf.get(Config.TRANSACTIONAL_ZOOKEEPER_SERVERS)) {
+ for (String server : (List<String>) stateConf.get(TRANSACTIONAL_ZOOKEEPER_SERVERS)) {
--- End diff --
I really don't understand this. Why not use Config? Is it because the Heron does not have a copy of org.apache.storm.Config on the classpath? Wouldn't that be the better solutions for compatibility, especially for something core to storm like the transactional zookeeper?
---
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: Force to be compatible with heron and add test...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1056#discussion_r51266910
--- Diff: external/storm-kafka/src/test/storm/kafka/KafkaTopologySpoutTestClient.java ---
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package storm.kafka;
+
+import backtype.storm.Config;
+import backtype.storm.LocalCluster;
+import backtype.storm.generated.StormTopology;
+import backtype.storm.spout.SchemeAsMultiScheme;
+import backtype.storm.task.OutputCollector;
+import backtype.storm.task.TopologyContext;
+import backtype.storm.topology.OutputFieldsDeclarer;
+import backtype.storm.topology.TopologyBuilder;
+import backtype.storm.topology.base.BaseRichBolt;
+import backtype.storm.tuple.Tuple;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+import java.util.Properties;
+import java.util.UUID;
+
+public class KafkaTopologySpoutTestClient {
+
+ private static StormTopology buildTopology(String zKeeperHost) {
+ ZkHosts zkHosts = new ZkHosts(zKeeperHost);
+ SpoutConfig spoutConfig = new SpoutConfig(zkHosts, "test", "/test", UUID.randomUUID().toString());
+ spoutConfig.scheme = new SchemeAsMultiScheme(new StringScheme());
+ KafkaSpout kafkaSpout = new KafkaSpout(spoutConfig);
+
+ TopologyBuilder builder = new TopologyBuilder();
+ builder.setSpout("kafkaSpout", kafkaSpout);
+
+ Properties props = new Properties();
+ props.put("zookeeper.connect", zKeeperHost);
+ props.put("acks", "1");
+ props.put("key.serializer", "org.apache.kafka.common.serialization.StringSerializer");
+ props.put("value.serializer", "org.apache.kafka.common.serialization.StringSerializer");
+
+ KafkaBolt bolt = new KafkaBolt();
+ builder.setBolt("kafkaBolt", bolt).shuffleGrouping("kafkaSpout");
+
+ return builder.createTopology();
+ }
+
+ /**
+ * To run this topology ensure you have a kafka broker running and provide connection string to broker as argument.
+ * Create a topic test with command line,
+ * kafka-topics.sh --create --zookeeper localhost:2181 --replication-factor 1 --partition 1 --topic test
+ * <p>
+ * run this program and run the kafka consumer:
+ * kafka-console-consumer.sh --zookeeper localhost:2181 --topic test --from-beginning
+ * <p>
+ * you should see the messages flowing through.
+ *
+ * @param args
+ * @throws Exception
+ */
+ public static void main(String[] args) throws Exception {
+ if (args.length < 1) {
+ System.out.println("Please provide zookeeper host,e.g. localhost:2181");
+ System.exit(1);
+ }
+
+ LocalCluster cluster = new LocalCluster();
+ cluster.submitTopology("kafkaTopologySpoutTest", new Config(), buildTopology(args[0]));
+ Thread.sleep(30 * 1000);
+ cluster.killTopology("kafkaTopologySpoutTest");
+
+ cluster.shutdown();
+ }
--- End diff --
Because this is using a local mode cluster could you please modify this to be a true JUnit test and spin up a separate instance of in-process zookeeper to act as the transactional zookeeper instance?
---
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.
---