[GitHub] storm pull request #2649: [STORM-3043] Fix NullPointerException when apply()...

[cleroux]

GitHub user cleroux opened a pull request:

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

[STORM-3043] Fix NullPointerException when apply() returns null

A null List is a valid return value from RecordTranslator.apply() if the 
ConsumerRecord is invalid.

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

$ git pull https://github.com/cleroux/storm 1.x-branch

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

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


commit 6b527dfc2b3ae7ba2c7e535e983fc8864f506133
Author: cleroux 
Date:   2018-04-27T22:36:05Z

[STORM-3043] Fix NullPointerException when apply() returns null




---

[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2638
  
@kishorvpatil Can you elaborate a bit? I don't understand why 
`TimeoutException` would be wrapped in an `InterruptedException`? Where is the 
log line you posted coming from, a grep of Storm didn't turn anything up?


---

[GitHub] storm pull request #2645: STORM-3042 fix acker and metric component resource...

[agresch]

Github user agresch closed the pull request at:

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


---

[GitHub] storm pull request #2648: STORM-3013: Keep KafkaConsumer open when storm-kaf...

[srdo]

GitHub user srdo opened a pull request:

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

STORM-3013: Keep KafkaConsumer open when storm-kafka-client spout is …

…deactivated, in order to keep metrics working

See https://issues.apache.org/jira/browse/STORM-3013

The changes here are to fix a crash that occur if the spout is deactivated 
and Storm asks for metrics. The KafkaConsumer is closed when the spout 
deactivates, and the metrics provider tries to use the consumer to generate a 
few metrics.

The suggested fix is to not close the KafkaConsumer when deactivating the 
spout. There's no real reason to do it as far as I can tell, and having to 
replace the consumer makes the spout a bit more complex than it would otherwise 
be.

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

$ git pull https://github.com/srdo/storm STORM-3013

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

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


commit ca506b82149745fdada5d4e9a6d41eaf958a8459
Author: Stig Rohde Døssing 
Date:   2018-03-30T17:12:01Z

STORM-3013: Keep KafkaConsumer open when storm-kafka-client spout is 
deactivated, in order to keep metrics working




---

[GitHub] storm issue #2642: [STORM-3043] Fix NullPointerException when apply() return...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2642
  
+1, thanks @cleroux, merged to master. Please open another PR against 
1.x-branch, so this fix can make it into 1.x releases too :)


---

[GitHub] storm pull request #2642: [STORM-3043] Fix NullPointerException when apply()...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] storm issue #2643: STORM-3039 handle slot ports in TIME_WAIT state

[raghavgautam]

Github user raghavgautam commented on the issue:

https://github.com/apache/storm/pull/2643
  
@ghajos We can bind to the port then close the port and then bind to the 
same port again. We have unit tests that can be used as a starting point.

https://github.com/apache/storm/blob/dd16ff2956c17707ab5637dd21a695daff7301cc/storm-core/test/clj/org/apache/storm/messaging/netty_unit_test.clj#L71


---

[GitHub] storm issue #2643: STORM-3039 handle slot ports in TIME_WAIT state

[ghajos]

Github user ghajos commented on the issue:

https://github.com/apache/storm/pull/2643
  
@raghavgautam I think it is out of scope to test a tcp testing in Storm. Do 
you have an idea how to do that?


---

[GitHub] storm pull request #2633: STORM-3028 HdfsSpout: handle empty file in case of...

[srdo]

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

https://github.com/apache/storm/pull/2633#discussion_r184707886
  
--- Diff: 
external/storm-hdfs/src/main/java/org/apache/storm/hdfs/spout/HdfsSpout.java ---
@@ -101,6 +101,7 @@
 private final AtomicBoolean commitTimeElapsed = new 
AtomicBoolean(false);
 private Timer commitTimer;
 private boolean fileReadCompletely = true;
+private boolean newReader = false;
--- End diff --

Does this need to be a field? It looks to me like it would work fine as a 
local too?


---

[GitHub] storm pull request #2633: STORM-3028 HdfsSpout: handle empty file in case of...

[srdo]

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

https://github.com/apache/storm/pull/2633#discussion_r184711553
  
--- Diff: 
external/storm-hdfs/src/test/java/org/apache/storm/hdfs/spout/TestHdfsSpout.java
 ---
@@ -153,6 +153,35 @@ public void testSimpleText_ACK() throws Exception {
 }
 }
 
+@Test
+public void testEmptySimpleText_ACK() throws Exception {
+Path file1 = new Path(source.toString() + "/file_empty.txt");
+createTextFile(file1, 0);
+
+Path file2 = new Path(source.toString() + "/file.txt");
+createTextFile(file2, 5);
+
+try (AutoCloseableHdfsSpout closeableSpout = 
makeSpout(Configs.TEXT, TextFileReader.defaultFields)) {
+HdfsSpout spout = closeableSpout.spout;
+spout.setCommitFrequencyCount(1);
+spout.setCommitFrequencySec(1);
+
+Map conf = getCommonConfigs();
+conf.put(Config.TOPOLOGY_ACKER_EXECUTORS, "1"); // enable 
ACKing
+openSpout(spout, 0, conf);
+
+// consume empty file
+runSpout(spout, "r6");
--- End diff --

Nit: Isn't it enough to call nextTuple once rather than 6 times here?


---

[GitHub] storm pull request #2633: STORM-3028 HdfsSpout: handle empty file in case of...

[srdo]

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

https://github.com/apache/storm/pull/2633#discussion_r184710659
  
--- Diff: 
external/storm-hdfs/src/test/java/org/apache/storm/hdfs/spout/TestHdfsSpout.java
 ---
@@ -153,6 +153,35 @@ public void testSimpleText_ACK() throws Exception {
 }
 }
 
+@Test
+public void testEmptySimpleText_ACK() throws Exception {
+Path file1 = new Path(source.toString() + "/file_empty.txt");
+createTextFile(file1, 0);
+
+Path file2 = new Path(source.toString() + "/file.txt");
+createTextFile(file2, 5);
+
+try (AutoCloseableHdfsSpout closeableSpout = 
makeSpout(Configs.TEXT, TextFileReader.defaultFields)) {
+HdfsSpout spout = closeableSpout.spout;
+spout.setCommitFrequencyCount(1);
--- End diff --

Is there any reason to set both commit frequency count and commit frequency 
sec? 


---

[GitHub] storm issue #2635: [STORM-3029] don't use keytab based login for hbase if Au...

[Ethanlm]

Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2635
  
rebased


---

[GitHub] storm issue #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_WORKERS ...

[Ethanlm]

Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2634
  
rebased.


---

[GitHub] storm issue #2643: STORM-3039 handle slot ports in TIME_WAIT state

[raghavgautam]

Github user raghavgautam commented on the issue:

https://github.com/apache/storm/pull/2643
  
Can you please add a test for this ?


---

[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2636
  
@cfriaszapater 
Thanks for the contribution. Basically we receive PRs which go first to the 
master branch, and additional PRs (against 1.x-branch) which is expected to not 
cleanly ported back. 
Could you craft a pull request against master branch?


---

[GitHub] storm issue #2646: [STORM-3044] AutoTGT should check if a TGT is address-les...

[Ethanlm]

Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2646
  
@HeartSaVioR  Thanks very much!


---

[GitHub] storm issue #2646: [STORM-3044] AutoTGT should check if a TGT is address-les...

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2646
  
@Ethanlm 
You can refer the DEVELOPER doc which guides contributors as well as 
committers. 

https://github.com/apache/storm/blob/master/DEVELOPER.md#merge-a-pull-request-or-patch
You would want to read BYLAWS for Storm project as well: 
http://storm.apache.org/contribute/BYLAWS.html

Please keep in mind we need to wait for 24hrs to merge code change. :) So 
you would like to try it out after 5 hours later.


---

[GitHub] storm issue #2646: [STORM-3044] AutoTGT should check if a TGT is address-les...

[Ethanlm]

Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2646
  
@HeartSaVioR  Sorry I forgot to change it. I don't see any merge button. Do 
I need to do it in some other way


---

[GitHub] storm issue #2633: STORM-3028 HdfsSpout: handle empty file in case of ackers

[ghajos]

Github user ghajos commented on the issue:

https://github.com/apache/storm/pull/2633
  
@srdo Can you please take a look at?


---

[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality

[cfriaszapater]

Github user cfriaszapater commented on the issue:

https://github.com/apache/storm/pull/2636
  
Ah, that's strange, because I did not change anything out of 
external/storm-cassandra. So that should be failing before this pull request. 
Anyway, I'll try what you say, thanks.


---