[GitHub] storm pull request: STORM-1366. Add documentation for StormSQL int...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/931#discussion_r47147496
  
--- Diff: documentation/storm-sql.md ---
@@ -0,0 +1,87 @@
+---
+title: Storm SQL integration
+layout: documentation
+documentation: true
+---
+
+The Storm SQL integration allows users to run SQL queries over streaming 
data in Storm. Not only the SQL interface allows faster development cycles on 
streaming analytics, but also opens up the opportunities to unify batch data 
processing like [Apache Hive](///hive.apache.org) and real-time streaming data 
analytics.
+
+At a very high level StormSQL compiles the SQL queries to 
[Trident](Trident-API-Overview.html) topologies and executes them in Storm 
clusters. This document provides information of how to use StormSQL as end 
users. For people that are interested in more details in the design and the 
implementation of StormSQL please refer to the [this](storm-sql-internal.html) 
page.
+
+## Usage
+
+Run the ``storm sql`` command to compile SQL statements into Trident 
topology, and submit it to the Storm cluster
+
+```
+$ bin/storm sql  
+```
+
+In which `sql-file` contains a list of SQL statements to be executed, and 
`topo-name` is the name of the topology.
+
+
+## Supported Features
+
+The following features are supported in the current repository:
+
+* Streaming from and to external data sources
+* Filtering tuples
+* Projections
+
+## Specifying External Data Sources
+
+In StormSQL data is represented by external tables. Users can specify data 
sources using the `CREATE EXTERNAL TABLE` statement. For example, the following 
statement specifies a Kafka spouts and sink:
+
+```
+CREATE EXTERNAL TABLE FOO (ID INT PRIMARY KEY) LOCATION 
'kafka://localhost:2181/brokers?topic=test' TBLPROPERTIES 
'{"producer":{"bootstrap.servers":"localhost:9092","acks":"1","key.serializer":"storm.kafka.IntSerializer","value.serializer":"storm.kafka.ByteBufferSerializer"}}'
+```
+
+The syntax of `CREATE EXTERNAL TABLE` closely follows the one defined in 
[Hive Data Definition 
Language](https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL).
+
+## Plugging in External Data Sources
+
+Users plug in external data sources through implementing the 
`ISqlTridentDataSource` interface and registers them using the mechanisms of 
Java's service loader. The external data source will be chosen based on the 
scheme of the URI of the tables. Please refer to the implementation of 
`storm-sql-kafka` for more details.
+
+## Example: Filtering Kafka Stream
+
+Let's say there is a Kafka stream that represents the transactions of 
orders. Each message in the stream contains the id of the order, the unit price 
of the product and the quantity of the orders. The goal is to filter orders 
where the transactions are significant and to insert these orders into another 
Kafka stream for further analysis.
+
+The user can specify the following SQL statements in the SQL file:
+
+```
+CREATE EXTERNAL TABLE ORDERS (ID INT PRIMARY KEY, UNIT_PRICE INT, QUANTITY 
INT) LOCATION 'kafka://localhost:2181/brokers?topic=orders' TBLPROPERTIES 
'{"producer":{"bootstrap.servers":"localhost:9092","acks":"1","key.serializer":"storm.kafka.IntSerializer","value.serializer":"storm.kafka.ByteBufferSerializer"}}'
+
+CREATE EXTERNAL TABLE LARGE_ORDERS (ID INT PRIMARY KEY, TOTAL INT) 
LOCATION 'kafka://localhost:2181/brokers?topic=large_orders' TBLPROPERTIES 
'{"producer":{"bootstrap.servers":"localhost:9092","acks":"1","key.serializer":"storm.kafka.IntSerializer","value.serializer":"storm.kafka.ByteBufferSerializer"}}'
+
+INSERT INTO LARGE_ORDERS SELECT ID, UNIT_PRICE * QUANTITY AS TOTAL FROM 
ORDERS WHERE UNIT_PRICE * QUANTITY > 50
+```
+
+The first statement defines the table `ORDER` which represents the input 
stream. The `LOCATION` clause specifies the ZkHost (`localhost:2181`), the path 
of the brokers in ZooKeeper (`/brokers`) and the topic (`orders`). The 
`TBLPROPERTIES` clause specifies the configuration of 
[KafkaProducer](http://kafka.apache.org/documentation.html#newproducerconfigs).
+Current implementation of `storm-sql-kafka` requires specifying both 
`LOCATION` and `TBLPROPERTIES` clauses even though the table is read-only or 
write-only.
+
+Similarly, the second statement specifies the table `LARGE_ORDERS` which 
represents the output stream. The third statement is a `SELECT` statement which 
defines the topology: it instructs StormSQL to filter a

[GitHub] storm pull request: STORM-1395: move JUnit dependency to top-level...

[ptgoetz]

GitHub user ptgoetz opened a pull request:

https://github.com/apache/storm/pull/950

STORM-1395: move JUnit dependency to top-level pom

https://issues.apache.org/jira/browse/STORM-1395

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ptgoetz/storm STORM-1395

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/950.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 #950


commit c89e4522164bb71c2b8805db02e67492b59dea71
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2015-12-16T20:59:07Z

STORM-1395: move JUnit dependency to top-level pom




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1399: Blobstore tests should write data ...

[ptgoetz]

GitHub user ptgoetz opened a pull request:

https://github.com/apache/storm/pull/955

STORM-1399: Blobstore tests should write data to target so it gets re…

…moved when running mvn clean

https://issues.apache.org/jira/browse/STORM-1399

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ptgoetz/storm STORM-1399

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/955.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 #955


commit 647012319d141564ad08fd1727e67675b0c2efed
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2015-12-17T15:56:05Z

STORM-1399: Blobstore tests should write data to target so it gets removed 
when running mvn clean




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1399: Blobstore tests should write data ...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/955#discussion_r47971375
  
--- Diff: 
external/storm-hdfs/src/test/java/org/apache/storm/hdfs/blobstore/BlobStoreTest.java
 ---
@@ -76,6 +76,7 @@
 
   @Before
   public void init() {
+System.setProperty("test.build.data", "target/test/data");
 initializeConfigs();
 baseFile = new File("/tmp/blob-store-test-"+UUID.randomUUID());
--- End diff --

It breaks the blob store tests, and chasing down the root cause was getting 
to be a pain. At least writing test data to /tmp doesn't clutter up the source 
tree, but I would also like to avoid that as well. I figured we could address 
that separately.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1418] improve debug logs for some exter...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/976#issuecomment-168802302
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1397: Merge conflict from Pacemaker merg...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/951#issuecomment-168808016
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1199 : HDFS Spout Functionally complete....

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/936#discussion_r48782534
  
--- Diff: pom.xml ---
@@ -204,7 +204,7 @@
 0.2.4
 3.3.2
 0.9.0
-16.0.1
+15.0
--- End diff --

Why the downgrade of the guava version?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1422 Remove broken example from the tuto...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/979#issuecomment-168794540
  
@d2r I'm fine with updating the instructions. If I remember correctly (it's 
been a while ;) ), it was to make it very easy for new users to see a topology 
run in local mode.

+1 for the 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1413] remove unused variables for some ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/974#issuecomment-168803581
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1401] removes multilang-test

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/966#issuecomment-168805521
  
+1

@d2r Did you create a follow-up JIRA for new multi-lang tests?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1419] Solr bolt should handle tick tupl...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/977#issuecomment-168801983
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1424] Removed unused topology-path vari...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/981#issuecomment-168792176
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: Update pom.xml

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/599#issuecomment-168829281
  
-1 flux-wrappers add convenience multi-lang spout/bolt implementations. 
Arguably, they could be included in flux-core. I'd rather go that route than 
set flux-wrappers to test scope, which will make them unavailable for people 
who want them.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1028: Eventhub spout meta data

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/651#issuecomment-168831787
  
@mooso Any update?

@rsltrifork Would you be willing to make the change/PR based on the earlier 
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 this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-706 . Clarify examples README.

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/465#issuecomment-168825799
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/885#issuecomment-168816433
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: add cgroup function that can limit cpu share o...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/667#issuecomment-168877269
  
I agree with @d2r, or would at least answer that, yes, we should hold off 
on cgroups support until the jstorm merge. I believe that @revans2 might have 
already covered this in JIRA. If not, please open a new issue.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-468] java.io.NotSerializableException s...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/477#issuecomment-168881795
  
Ping (mostly to other committers to solicit review/discussion).

This is not a bad idea.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-553:add eclipse java formatter file for ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/478#issuecomment-168884966
  
I'm all for adopting a code style guide for all languages (Java, Clojure, 
Python, etc.), but I'd rather see a style specification documented first, then 
follow up with templates that implement it.

I do feel that we have a lot of code style technical debt in both 
Java/Clojure. If we're moving away from clojure, we don't need to worry about 
style too much there, but we should establish a style guide for Java source, 
and start cleaning up compiler warnings, PMD/Checkstyle/Findbugs issues, etc.

I would suggest a discussion on the dev@ list, and hope it doesn't become 
too religious. ;)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1425] Tick tuples must be acked like no...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/982#issuecomment-169065098
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1417: fixed equals/hashCode contract in ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/975#issuecomment-169068835
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1348 - refactor API to remove Insert/Upd...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/929#issuecomment-169060207
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r48987203
  
--- Diff: external/storm-mqtt/core/pom.xml ---
@@ -0,0 +1,153 @@
+
+http://maven.apache.org/POM/4.0.0; 
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
+  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+  4.0.0
+
+  storm-mqtt
+  jar
+
+  storm-mqtt
+
+  
+org.apache.storm
+storm-mqtt-parent
+  0.11.0-SNAPSHOT
+../pom.xml
+  
+
+  
+UTF-8
+  
+
+  
+
+  bintray
+  http://dl.bintray.com/andsel/maven/
+  
+true
+  
+  
+false
+  
+
+  
+
+  
+  
+  org.apache.activemq
+  activemq-broker
+  5.9.0
+  test
+  
+  
+  org.apache.activemq
+  activemq-mqtt
+  5.9.0
+  test
+  
+  
+  org.apache.activemq
+  activemq-kahadb-store
+  5.9.0
+  test
+  
+
+  
+  org.apache.storm
+  storm-core
+  ${project.version}
+  provided
+
+
+  org.apache.storm
+  flux-core
+  ${project.version}
+
+
+  org.fusesource.mqtt-client
+  mqtt-client
+  1.10
+
+
--- End diff --

Good catch. That a remnant from the original example that used HDFS.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r48976202
  
--- Diff: 
external/storm-mqtt/core/src/main/java/org/apache/storm/mqtt/common/MqttUtils.java
 ---
@@ -0,0 +1,44 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.mqtt.common;
+
+
+import org.fusesource.mqtt.client.QoS;
+
+public class MqttUtils {
+
+private MqttUtils(){}
+
+public static QoS qosFromInt(int i){
+QoS qos = null;
+switch(i) {
+case 0:
+qos = QoS.AT_MOST_ONCE;
+break;
+case 1:
+qos = QoS.AT_LEAST_ONCE;
+break;
+case 2:
+qos = QoS.EXACTLY_ONCE;
--- End diff --

I wanted to shield users from the underlying MQTT client implementation. 
The `QoS` enum is in the `org.fusesource.mqtt.client` package, which users 
would have to import. If for some reason we switched the MQTT client 
implementation, it would break existing user code. With this approach we can 
avoid that.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r48976235
  
--- Diff: 
external/storm-mqtt/core/src/main/java/org/apache/storm/mqtt/common/MqttOptions.java
 ---
@@ -0,0 +1,334 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.mqtt.common;
+
+import java.io.Serializable;
+import java.util.List;
+
+/**
+ * MQTT Configuration Options
+ */
+public class MqttOptions implements Serializable {
+private String url = "tcp://localhost:1883";
+private List topics = null;
+private boolean cleanConnection = false;
+
+private String willTopic;
+private String willPayload;
+private int willQos = 1;
+private boolean willRetain = false;
+
+private long reconnectDelay = 10;
+private long reconnectDelayMax = 30*1000;
+private double reconnectBackOffMultiplier = 2.0f;
+private long reconnectAttemptsMax = -1;
+private long connectAttemptsMax = -1;
+
+private String userName = "";
+private String password = "";
+
+private int qos = 1;
--- End diff --

See above comment.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r48976364
  
--- Diff: 
external/storm-mqtt/core/src/main/java/org/apache/storm/mqtt/MqttLogger.java ---
@@ -0,0 +1,45 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.mqtt;
+
+import org.fusesource.mqtt.client.Tracer;
+import org.fusesource.mqtt.codec.MQTTFrame;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Wrapper around SLF4J logger that allows MQTT messages to be logged.
+ */
+public class MqttLogger extends Tracer {
+private static final Logger LOG = 
LoggerFactory.getLogger(MqttLogger.class);
+
+@Override
+public void debug(String message, Object... args) {
+LOG.debug(String.format(message, args));
+}
+
+@Override
+public void onSend(MQTTFrame frame) {
+super.onSend(frame);
--- End diff --

Will do.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r48976420
  
--- Diff: 
external/storm-mqtt/core/src/main/java/org/apache/storm/mqtt/bolt/MqttBolt.java 
---
@@ -0,0 +1,108 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.mqtt.bolt;
+
+import backtype.storm.Config;
+import backtype.storm.task.OutputCollector;
+import backtype.storm.task.TopologyContext;
+import backtype.storm.topology.OutputFieldsDeclarer;
+import backtype.storm.topology.base.BaseRichBolt;
+import backtype.storm.tuple.Tuple;
+import backtype.storm.utils.TupleUtils;
+import org.apache.storm.mqtt.MqttMessage;
+import org.apache.storm.mqtt.common.MqttOptions;
+import org.apache.storm.mqtt.MqttTupleMapper;
+import org.apache.storm.mqtt.common.MqttPublisher;
+import org.apache.storm.mqtt.common.SslUtils;
+import org.apache.storm.mqtt.ssl.KeyStoreLoader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+
+
+public class MqttBolt extends BaseRichBolt {
+private static final Logger LOG = 
LoggerFactory.getLogger(MqttBolt.class);
+private MqttTupleMapper mapper;
+private transient MqttPublisher publisher;
+private boolean retain = false;
+private transient OutputCollector collector;
+private MqttOptions options;
+private KeyStoreLoader keyStoreLoader;
+private transient String topologyName;
+
+
+public MqttBolt(MqttOptions options, MqttTupleMapper mapper){
+this(options, mapper, null, false);
+}
+
+public MqttBolt(MqttOptions options, MqttTupleMapper mapper, boolean 
retain){
+this(options, mapper, null, retain);
+}
+
+public MqttBolt(MqttOptions options, MqttTupleMapper mapper, 
KeyStoreLoader keyStoreLoader){
+this(options, mapper, keyStoreLoader, false);
+}
+
+public MqttBolt(MqttOptions options, MqttTupleMapper mapper, 
KeyStoreLoader keyStoreLoader, boolean retain){
+this.options = options;
+this.mapper = mapper;
+this.retain = retain;
+this.keyStoreLoader = keyStoreLoader;
+// the following code is duplicated in the constructor of 
MqttPublisher
+// we reproduce it here so we fail on the client side if SSL is 
misconfigured, rather than when the topology
+// is deployed to the cluster
+SslUtils.checkSslConfig(this.options.getUrl(), keyStoreLoader);
+}
+
+@Override
+public void prepare(Map conf, TopologyContext context, OutputCollector 
collector) {
+this.collector = collector;
+this.topologyName = (String)conf.get(Config.TOPOLOGY_NAME);
+this.publisher = new MqttPublisher(this.options, 
this.keyStoreLoader, this.retain);
+try {
+this.publisher.connectMqtt(this.topologyName + "-" + 
context.getThisComponentId() + "-" + context.getThisTaskId());
+} catch (Exception e) {
+LOG.error("Unable to connect to MQTT Broker.", e);
+throw new RuntimeException("Unable to connect to MQTT 
Broker.", e);
+}
+}
+
+@Override
+public void execute(Tuple input) {
+//ignore tick tuples
+if(!TupleUtils.isTick(input)){
+MqttMessage message = this.mapper.toMessage(input);
+try {
+this.publisher.publish(message, this.retain);
+this.collector.ack(input);
+} catch (Exception e) {
+LOG.warn("Error publishing MQTT message. Failing tuple.", 
e);
+// should we fail the tuple or kill the worker?
--- End diff --

Good call.


---
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

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r49160828
  
--- Diff: 
external/storm-mqtt/core/src/main/java/org/apache/storm/mqtt/trident/MqttPublishFunction.java
 ---
@@ -0,0 +1,84 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.mqtt.trident;
+
+import backtype.storm.Config;
+import backtype.storm.task.OutputCollector;
+import backtype.storm.topology.FailedException;
+import org.apache.storm.mqtt.MqttMessage;
+import org.apache.storm.mqtt.common.MqttOptions;
+import org.apache.storm.mqtt.MqttTupleMapper;
+import org.apache.storm.mqtt.common.MqttPublisher;
+import org.apache.storm.mqtt.common.SslUtils;
+import org.apache.storm.mqtt.ssl.KeyStoreLoader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import storm.trident.operation.BaseFunction;
+import storm.trident.operation.TridentCollector;
+import storm.trident.operation.TridentOperationContext;
+import storm.trident.tuple.TridentTuple;
+
+import java.util.Map;
+
+public class MqttPublishFunction extends BaseFunction {
--- End diff --

I thought about that distinction. Ultimately, I decided upon a function 
based on the following criteria:

1, There is no bulk publishing mechanism with MQTT, so there wouldn't be 
any performance benefit in processing tuples in batch.
2. Aside from publishing a message with a QoS of 2, we don't have any way 
to truly determine if it reached all subscribers, only the broker, and if the 
broker dies...??
3. Exactly once is virtually impossible with MQTT in a distributed 
environment. 
4. Most MQTT use cases that need to scale avoid QoS 2 because of the 
overhead.

That being said, I'm perfectly open to exploring a MQTT State 
implementation if it would be useful.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/991#issuecomment-170105203
  
@satishd, @arunmahadevan, @HeartSaVioR Thanks for your reviews. I believe 
I've addressed all your comments. I've also added documentation.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

GitHub user ptgoetz opened a pull request:

https://github.com/apache/storm/pull/991

STORM-1406: Add MQTT Support

https://issues.apache.org/jira/browse/STORM-1406

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ptgoetz/storm storm-mqtt

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/991.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 #991


commit f157ab423f6a539d07c512fac4dd97734e3d62e5
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2016-01-05T21:49:15Z

STORM-1406: Add MQTT Support




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/991#issuecomment-169145828
  
Initial commit for code review. Further documentation to follow.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1199 : HDFS Spout Functionally complete....

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/936#issuecomment-168812437
  
Minor nit, but several instances throughout:

Can you update the style so that all `if` blocks are enclosed in `{}` (even 
if it is only one line)?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1412] null check should be done in the ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/970#issuecomment-168813390
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1427] add TupleUtils/listHashCode metho...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/985#issuecomment-168754927
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1406: Add MQTT Support

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/991#discussion_r49006394
  
--- Diff: 
external/storm-mqtt/core/src/main/java/org/apache/storm/mqtt/common/MqttUtils.java
 ---
@@ -0,0 +1,44 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.mqtt.common;
+
+
+import org.fusesource.mqtt.client.QoS;
+
+public class MqttUtils {
+
+private MqttUtils(){}
+
+public static QoS qosFromInt(int i){
+QoS qos = null;
+switch(i) {
+case 0:
+qos = QoS.AT_MOST_ONCE;
+break;
+case 1:
+qos = QoS.AT_LEAST_ONCE;
+break;
+case 2:
+qos = QoS.EXACTLY_ONCE;
--- End diff --

Those number are part of the MQTT protocol and are well known.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1408] clean up the build directory crea...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/964#issuecomment-166693980
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: Fix blobstore log class

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/962#issuecomment-16775
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1218: Use markdown for JavaDoc

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/891#issuecomment-159422903
  
**Note:** This pull request originally included the following commit: 
https://github.com/apache/storm/commit/e03b28ca6222f61471b4acb1a6086aab9b597b8a

Earlier today I inadvertently pushed that commit to master. I have since 
reverted it in master. The last commit in this PR re-adds what was reverted, 
but only in my repository.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1075 add external module storm-cassandra

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/827#issuecomment-159353605
  
@satishd I think that work can be done in follow-up JIRAs after this PR is 
merged. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1207: Added flux support for IWindowedBo...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/899#issuecomment-159365011
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1075 add external module storm-cassandra

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/827#issuecomment-159331571
  
@fhussonnois I think that sounds like a good approach.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1218: Use markdown for JavaDoc

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/891#issuecomment-159371834
  
I removed the highlight.js license from the source release LICENSE file 
since generated javadoc is not part of a source release.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: Update ObjectDef.java for flux

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/732#issuecomment-159378631
  
+1 tests can be added 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1218: Use markdown for JavaDoc

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/891#issuecomment-157928854
  
Re: Merge conflicts, I'm happy to resolve therm at merge time.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-885] Heartbeat Server (Pacemaker)

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/838#issuecomment-157914621
  
@knusbaum Nice documentation! Thank you.

Some thoughts on documentation improvements (please correct me if I'm 
wrong):

1. Since pacemaker is a single node service, what happens in the event it 
goes down or gets separated by a network partition?
2. How does it compare to the default zookeeper-backed implementation 
(performance, fault tolerance, how requirements, etc.).

Documentation can make or break adoption of a new feature/API/etc.

I'm +1 for merging this with the current documentation. +2 if the points 
above are addresses or a follow-up JIRA is created.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1218: Use markdown for JavaDoc

[ptgoetz]

GitHub user ptgoetz opened a pull request:

https://github.com/apache/storm/pull/891

STORM-1218: Use markdown for JavaDoc

JIRA: https://issues.apache.org/jira/browse/STORM-1218

This is a first pass at converting the JavaDoc in `storm-core` to markdown, 
using [pegdown-doclet](https://github.com/Abnaxos/pegdown-doclet). If there is 
support from the community, I will move on to other components, though any html 
will just pass through, so it shouldn't garble, and will likely improve any 
existing javadoc.

The first commit adds the corresponding maven javadoc plugin to the default 
build. This means it will increase build time when you run `mvn install`, but 
it will also spit out warnings about javadoc problems and may encourage 
developers to fix or improve documentation problems.

The second commit is a quick pass over `storm-core` to remove unnecessary 
HTML and some minor javadoc comment linting. Only javadoc comments were 
modified.

The third commit adds the `highlight.js` license to both the source and 
binary LICENSE files, since the generated javadoc includes it. I'm not sure 
this is necessary, since the JavaDoc is generated, but I will find out if 
there's support for this patch.

*Note:* The [pegdown-doclet 
licence](https://github.com/Abnaxos/pegdown-doclet/blob/master/LICENSE.txt) is 
GPL, but it's only used at build time, so that does not seem to present a 
licensing issue.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ptgoetz/storm markdown-javadoc

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/891.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 #891


commit e03b28ca6222f61471b4acb1a6086aab9b597b8a
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2015-11-19T01:16:47Z

enable markdown in javadoc

commit d6d85518bc7a668dfe7cad9e796c226752ad0aca
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2015-11-19T01:19:09Z

convert existing javadoc to markdown

commit bc77f1f59622cff0e0bef01dfbc821ac3dba21c1
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2015-11-19T01:25:27Z

add highlight.js licesnse to LICENSE files




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/887#issuecomment-157880047
  
@revans2 Ah... okay. So we need to change `resources/resources` to just 
`resources`, and remove the `-1.6.4` from the libraries, correct?

That should be doable with the dependency plugin.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/887#issuecomment-158184985
  
@revans2 I couldn't get the maven dependency plugin to do what I wanted, so 
I changed tactics. I switched to using the maven antrun plugin to download and 
unpack the sigar native binaries. It downloads the archive to the local maven 
repository alongside the other sigar dependencies so it only needs to download 
it once.

The good news is that allowed me to use the full binary archive, so it 
includes support for windows and other OSes/architectures. On the down side 
it's somewhat brittle compared to using "pure" maven, and IMO is kind of ugly. 
It's also hard-coded to use google code, which is supposedly going away at some 
point, but that's the only place I could find the archive with a published 
checksum.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-885] Heartbeat Server (Pacemaker)

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/838#issuecomment-158193770
  
@knusbaum Nice work on the documentation. Thank you.

+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/887#issuecomment-157868784
  
@revans2 How are you running it (i.e. local/remote/unit test)? My first 
thought is that we may just need to bind the dependency plugin to a different 
maven lifecycle.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/887#issuecomment-157869384
  
@revans2 That directory structure is what's in master right now:


https://github.com/apache/storm/tree/master/external/storm-metrics/src/main/resources/resources


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1218: Use markdown for JavaDoc

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/891#issuecomment-158109736
  
@revans2 Yes, it is inserted into the generated HTML. Since javadoc isn't 
part of the source release, it probably only needs to be added to the LICENSE 
for the binary distribution.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1175] State store for windowing operati...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/939#issuecomment-171028002
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1176] Checkpoint window evaluated/expir...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/963#discussion_r49506460
  
--- Diff: 
examples/storm-starter/src/jvm/storm/starter/StatefulWindowingTopology.java ---
@@ -0,0 +1,110 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.starter;
+
+import org.apache.storm.Config;
+import org.apache.storm.LocalCluster;
+import org.apache.storm.StormSubmitter;
+import org.apache.storm.generated.StormTopology;
+import org.apache.storm.starter.bolt.PrinterBolt;
+import org.apache.storm.starter.spout.RandomIntegerSpout;
+import org.apache.storm.state.KeyValueState;
+import org.apache.storm.state.State;
+import org.apache.storm.task.OutputCollector;
+import org.apache.storm.task.TopologyContext;
+import org.apache.storm.topology.OutputFieldsDeclarer;
+import org.apache.storm.topology.TopologyBuilder;
+import org.apache.storm.topology.base.BaseStatefulWindowedBolt;
+import org.apache.storm.tuple.Fields;
+import org.apache.storm.tuple.Tuple;
+import org.apache.storm.tuple.Values;
+import org.apache.storm.utils.Utils;
+import org.apache.storm.windowing.TupleWindow;
+
+import java.util.Map;
+
+import static org.apache.storm.topology.base.BaseWindowedBolt.Count;
+
+/**
+ * A simple example that demonstrates the usage of {@link 
org.apache.storm.topology.IStatefulWindowedBolt} to
+ * save the state of the windowing operation to avoid re-computation in 
case of failures.
+ * 
+ * The framework internally manages the window boundaries and does not 
invoke
+ * {@link org.apache.storm.topology.IWindowedBolt#execute(TupleWindow)} 
for the already evaluated windows in case of restarts
+ * during failures. The {@link 
org.apache.storm.topology.IStatefulBolt#initState(State)}
+ * is invoked with the previously saved state of the bolt after prepare, 
before the execute() method is invoked.
+ * 
+ */
+public class StatefulWindowingTopology {
+
+private static class WindowSumBolt extends 
BaseStatefulWindowedBolt<KeyValueState<String, Long>> {
+private KeyValueState<String, Long> state;
+private long sum;
+
+private OutputCollector collector;
+
+@Override
+public void prepare(Map stormConf, TopologyContext context, 
OutputCollector collector) {
+this.collector = collector;
+}
+
+@Override
+public void initState(KeyValueState<String, Long> state) {
+this.state = state;
+sum = state.get("sum", 0L);
+System.out.println("initState with state [" + state + "] 
current sum [" + sum + "]");
--- End diff --

Could we use slf4j here instead of `System.out`?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-587. trident transactional state in zk s...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/475#issuecomment-171048834
  
Unless others feel strongly about this change, I think it's okay to close 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1176] Checkpoint window evaluated/expir...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/963#issuecomment-171044220
  
+1 aside from 2 very minor nits, but the feature is sizable so I'd like to 
see additional committer reviews.

I'd also like to see this included in the 1.0 release. It does touch 
clojure code (`executor.clj`), but in a very minor way.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1176] Checkpoint window evaluated/expir...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/963#discussion_r49507663
  
--- Diff: storm-core/src/jvm/org/apache/storm/state/StateProvider.java ---
@@ -0,0 +1,38 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 org.apache.storm.state;
+
+import org.apache.storm.task.TopologyContext;
+
+import java.util.Map;
+
+/**
+ * Used by the {@link StateFactory} to create a new state instances.
+ */
+public interface StateProvider {
+/**
+ * Returns a new state instance. Each state belongs unique namespace 
which is typically
+ * the componentid-task of the task, so that each task can have its 
own unique state.
+ *
+ * @param namespace a namespace of the state
+ * @param stormConf the storm topology configuration
+ * @param context   the {@link TopologyContext}
+ * @return a previously saved state instance
--- End diff --

Minor documentation nit: Doesn't it return a previously saved state if one 
exists, and otherwise return a new 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-919 Gathering worker and supervisor proc...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/608#issuecomment-171049571
  
@bourneagain Any update on 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1453] nimbus.clj/wait-for-desired-code-...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1003#issuecomment-171045749
  
@revans2 looks like this was originally #996, which had the requisite +1s, 
but needed an upmerge. I think @caofangkun just opened a new pull request and 
closed #996.

+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: Merge changes from {master}/docs to {asf-site}

[ptgoetz]

GitHub user ptgoetz opened a pull request:

https://github.com/apache/storm/pull/1008

Merge changes from {master}/docs to {asf-site}

JIRA: https://issues.apache.org/jira/browse/STORM-1468

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ptgoetz/storm asf-site

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/1008.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 #1008


commit d63146b7af0aa4db4af7c5f812ca5358f9395aee
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2016-01-12T22:31:44Z

update documentation




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1468: remove {master}/docs

[ptgoetz]

GitHub user ptgoetz opened a pull request:

https://github.com/apache/storm/pull/1009

STORM-1468: remove {master}/docs

JIRA: https://issues.apache.org/jira/browse/STORM-1468

The content of the {master}/docs directory is now in the {asa-site} branch.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ptgoetz/storm STORM-1468

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/1009.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 #1009


commit 2f5c31d24ccc2cb220732f8bc85c9218fbeda221
Author: P. Taylor Goetz <ptgo...@gmail.com>
Date:   2016-01-13T01:40:58Z

STORM-1468: remove {master}/docs




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1468: remove {master}/docs

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1009#issuecomment-171126186
  
Assuming lazy consensus.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1468: remove {master}/docs

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1009#issuecomment-171126632
  
See also #1008 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1468: Merge changes from {master}/docs t...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1008#issuecomment-171126410
  
Assuming lazy consensus, but would appreciate review in case I missed 
anything.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1468: Merge changes from {master}/docs t...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1008#issuecomment-171126503
  
See also #1009


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1456: [STORM-1874]Update logger private permissions

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1456
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1473: STORM-1864 : StormSubmitter should throw respective excep...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1473
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1324: STORM-1700 Introduce 'whitelist' / 'blacklist' option to ...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1324
  
See my comments on #1325 as they apply here as well.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #1325: STORM-1700 Introduce 'whitelist' / 'blacklist' opt...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/1325#discussion_r66292563
  
--- Diff: conf/storm.yaml.example ---
@@ -39,10 +39,13 @@
 # - "server2"
 
 ## Metrics Consumers
+## NOTE: task queue will be unbounded when max.retain.metric.tuples is 
equal or less than 0.
 # topology.metrics.consumer.register:
 #   - class: "org.apache.storm.metric.LoggingMetricsConsumer"
+# max.retain.metric.tuples: 100
 # parallelism.hint: 1
 #   - class: "org.mycompany.MyMetricsConsumer"
+# max.retain.metric.tuples: 100
--- End diff --

It would be helpful to have an example of how to specify include/exclude 
filters (whitelist/blacklist).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1471: STORM-1865: update command line client document

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1471
  
+1 nice job!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #1325: STORM-1700 Introduce 'whitelist' / 'blacklist' opt...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/1325#discussion_r66292249
  
--- Diff: storm-core/src/clj/org/apache/storm/daemon/common.clj ---
@@ -298,18 +299,21 @@
   {[comp-id METRICS-STREAM-ID] :shuffle})
 (into {}))
 
-mk-bolt-spec (fn [class arg p]
+mk-bolt-spec (fn [class arg p max-retain-metric-tuples whitelist 
blacklist]
(thrift/mk-bolt-spec*
 inputs
-(org.apache.storm.metric.MetricsConsumerBolt. 
class arg)
+(org.apache.storm.metric.MetricsConsumerBolt. 
class arg max-retain-metric-tuples (FilterByMetricName. whitelist blacklist))
--- End diff --

Are MetricsFilters intended to be pluggable, or do we only want to allow 
filtering by name?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1453: STORM-1873 Implement alternative behaviour for late tuple...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1453
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1379: STORM-1742 (1.x) More accurate 'complete latency'

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1379
  
I wouldn't mind seeing some larger-scale performance tests, since it 
doubles the size of ACK tuples (I know, they're still small). If others can 
confirm the absence of any performance degradation I'd be +1.

I'll try to test as well when I get a chance.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #1331: STORM-1705: Cap number of retries for a failed mes...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/1331#discussion_r66318933
  
--- Diff: 
external/storm-kafka/src/jvm/org/apache/storm/kafka/ExponentialBackoffMsgRetryManager.java
 ---
@@ -86,15 +94,23 @@ public Long nextFailedMessageToRetry() {
 }
 
 @Override
-public boolean shouldRetryMsg(Long offset) {
+public boolean shouldReEmitMsg(Long offset) {
 MessageRetryRecord record = this.records.get(offset);
 return record != null &&
 this.waiting.contains(record) &&
 System.currentTimeMillis() >= record.retryTimeUTC;
--- End diff --

I would be nice to log a WARN message when we are dropping messages so 
users have a way of knowing when/why it is happening.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #1331: STORM-1705: Cap number of retries for a failed mes...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/1331#discussion_r66318510
  
--- Diff: 
external/storm-kafka/src/jvm/org/apache/storm/kafka/SpoutConfig.java ---
@@ -37,6 +37,8 @@
 public long retryInitialDelayMs = 0;
 public double retryDelayMultiplier = 1.0;
 public long retryDelayMaxMs = 60 * 1000;
+public int retryLimit = Integer.MAX_VALUE;
--- End diff --

This is probably an unlikely edge case, but theoretically possible: If a 
tuple is retried `Integer.MAX_VALUE` times, it will be dropped.

To truly turn off retry limits, I would suggest setting this to `-1` and 
adding logic to check for that in logic that checks to see if the retry limit 
has been reached.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1331: STORM-1705: Cap number of retries for a failed message

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1331
  
Two minor nits. I'm +1 once they are addressed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1468: STORM-1885. python script for squashing and merging prs.

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1468
  
@harshach The source of the file is referenced here:

https://github.com/apache/storm/pull/1468/files#diff-da45fe3972445a9f82ef768808dd8853R20

I'd like to get clearance that what this script does or enables is okay 
before proceeding. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1464: STORM-1884: Prioritize pendingPrepare over pendingCommit

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1464
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1461: [STORM-1882] Expose TextFileReader public

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1461
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1468: STORM-1885. python script for squashing and merging prs.

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1468
  
> It will ask for primary authors and the user who is merging this can 
input more than one author at the time of merge.

That means it removes authorship information. If we tag a squashed commit 
as coming from multiple authors, we still wouldn't be able to differentiate 
what code was contributed by the individual authors.

So if I merged a pull request with multiple authors, the result would be a 
single commit from me with a message listing the contributing authors, is that 
correct?




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1458: STORM-1878: Flux can now handle IStatefulBolts

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1458
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1468: STORM-1885. python script for squashing and merging prs.

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1468
  
I'm a little on the fence in terms of squashing the commits of others vs. 
asking the contributor to do so. There are a lot of situations where spreading 
out a big patch over multiple commits makes sense and makes the history more 
consumable. 

A couple of questions:
* How does this preserve authorship in a pull request that has commits from 
multiple authors?
* How would this work with our current branch model? Specifically, applying 
a pull request to multiple branches.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1481: STORM-1705: Fix cap-retry bug

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1481
  
+1 Thanks @abhishekagarwal87.

Since this is a one-line change blocking the release, I'm going to merge it 
in the interest of proceeding with the release. If there are any objections we 
can cancel the next vote and revert the 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1325: STORM-1700 Introduce 'whitelist' / 'blacklist' option to ...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1325
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1324: STORM-1700 Introduce 'whitelist' / 'blacklist' option to ...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1324
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1467: STORM-1771. HiveState should flushAndClose before closing...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1467
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1467: STORM-1771. HiveState should flushAndClose before closing...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1467
  
There was a compilation issue with this patch. I fixed it with this commit: 
0b54767
Please review. If there are any objections I can revert.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1331: STORM-1705: Cap number of retries for a failed message

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1331
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1766] - A better algorithm server rack ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1398#issuecomment-221677136
  
@jerrypeng Did you merge this to any other branches, or just master?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1861 Fix storm script bug to not fork ja...

[ptgoetz]

Github user ptgoetz commented on a diff in the pull request:

https://github.com/apache/storm/pull/1441#discussion_r64637467
  
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
 extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
 args=args,
 daemon=False,
--- End diff --

Ah... that makes sense now. So how concerned are we about leaking a tmp 
file vs. allowing users to build tools around topology submission (and get the 
right exit code)?

Would it make sense to have a separate command to do the topology transform 
instead of in-lining it with topology submission? I.e. two steps: 1. transform 
topology, 2. submit topology jar output from step 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1766] - A better algorithm server rack ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1398#issuecomment-221686373
  
@jerrypeng For tracking what goes into each branch/release. Github only 
gives us merge notifications for the branch a pull request targeted. If you had 
merged this to other branches, we wouldn't know unless we looked for it in 
other branches. That's why most of the time we add a comment noting which 
branches a patch was applied to. It saves a little time for other committers.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1468: STORM-1885. python script for squashing and merging prs.

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1468
  
> We won't be able capture this in JIRA either. I am not sure how much of 
this is important to have all the commits from each contributor for a single 
JIRA which in itself is rare unless its a big patch. It does have ability to 
give each contributor credit in the commit log.

From a legal perspective it's very important that we be able to track the 
provenance of all code that lands in an ASF repository and could potentially be 
released.

For example: 

Bob is a committer. Alice and Charles are not. Alice and Charles 
collaborate on a patch, both making commits. In the process Charles commits 
some code that he doesn't have the legal rights to (its proprietary, etc.). 
Later Bob uses this script to merge the pull request, and squash all the 
commits. Alice and Charles are listed as authors of the patch, but there is no 
history regarding how the code that the ASF doesn't have rights to get there. 
Was it Charles or Alice?

That may seem like an edge case, but one that we should absolutely consider.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1153: [STORM-1575] fix TwitterSampleSpout NPE on close

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/1153
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1864 StormSubmitter should throw respect...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1446#issuecomment-221764594
  
+1 Thanks for the clarification @satishd!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1861 Fix storm script bug to not fork ja...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1448#issuecomment-222169053
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1868] Modify TridentKafkaWordCount to r...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1449#issuecomment-01561
  
+1 This should probably also be applied to the 1.* branches.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1466: Move the org.apache.thrift7 namesp...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1007#issuecomment-171365717
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1467: Switch apache-rat plugin off by de...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1006#issuecomment-171366113
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1452] Fixes profiling/debugging out of ...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1012#issuecomment-171816513
  
Works for me. Thanks @d2r.

+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1478] make bolt's getComponentConfigura...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1020#issuecomment-172728583
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---