[GitHub] storm issue #2637: Map of Spout configurations from `storm-kafka` to `storm-...

[srishtyagrawal]

Github user srishtyagrawal commented on the issue:

https://github.com/apache/storm/pull/2637
  
@srdo thanks for explaining that. I was looking at the GitHub file links 
that's why they were giving 404s.

I have also modified the `socketTimeoutMs` setting map to not be supported 
in `storm-kafka-client`. The previous translation was from [Kafka's 
ConsumerConfig.scala](https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/consumer/ConsumerConfig.scala#L30),
 but looks like the scala file is deprecated, also this configuration is only 
present in [old Kafka Consumer configs 
table](https://kafka.apache.org/documentation/#oldconsumerconfigs) and not in 
[new Kafka Consumer configs 
table](https://kafka.apache.org/documentation/#newconsumerconfigs).

According to your suggestion the Storm links have been modified to be 
generic hence not tied to a particular release version, but I also have links 
to Kafka properties which are still release specific (last release - 1.1). This 
is because kafka-site repo doesn’t do release agnostic links. 

Let me know if the table looks fine, I will then publish them to Storm docs.



---

[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/2518
  
+1


---

[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module

[hustfxj]

Github user hustfxj commented on the issue:

https://github.com/apache/storm/pull/2518
  
+1 Thank you for @vesense 


---

[GitHub] storm pull request #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_W...

[roshannaik]

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

https://github.com/apache/storm/pull/2634#discussion_r182909146
  
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/Worker.java 
---
@@ -305,8 +305,9 @@ private void setupFlushTupleTimer(final Map topologyConf, final
 }
 
 private void setupBackPressureCheckTimer(final Map 
topologyConf) {
-final Integer workerCount = 
ObjectReader.getInt(topologyConf.get(Config.TOPOLOGY_WORKERS));
-if (workerCount <= 1) {
+Set nonLocalTasks = 
Sets.difference(workerState.getTaskToComponent().keySet(),
--- End diff --

Thanks for clarifications. IMO, thats a nice (eventhough small) improvement 
to disable interworker when there are no outbound tasks... and not just for 
1worker mode. 

Minor request: would be nice to have named functions for those two set 
difference calls. I suspect they will start seeing more usage soon.

looks good thanks for taking care of this.



---

[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
  
Sorry if I'm nitpicking you to death, but I don't think moving the log line 
is better. Now everyone gets an error level log, even if the exception is due 
to an interrupt. I'd prefer if we just include the exception in both this log 
statement and the one in the interrupt case.


---

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

[kishorvpatil]

Github user kishorvpatil commented on the issue:

https://github.com/apache/storm/pull/2638
  
@srdo  Moved the `LOG.error` before changing exception Cause.


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[kishorvpatil]

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

https://github.com/apache/storm/pull/2638#discussion_r182847223
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -109,6 +109,8 @@
 
 import javax.security.auth.Subject;
 
+import static org.apache.commons.lang.exception.ExceptionUtils.*;
--- End diff --

Updated.


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[srdo]

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

https://github.com/apache/storm/pull/2638#discussion_r182844665
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -361,6 +363,7 @@ public void run() {
 Time.sleep(s);
 }
 } catch (Throwable t) {
+LOG.info("Async loop Exception Stacktrace is: {} ", 
getStackTrace(t));
--- End diff --

Thanks. Wouldn't it make sense to add the throwable to the log statement 
there then, so both L370 and 372 print the exception? (I'm asking why we want a 
separate log for this stack trace)


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[kishorvpatil]

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

https://github.com/apache/storm/pull/2638#discussion_r182843808
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -361,6 +363,7 @@ public void run() {
 Time.sleep(s);
 }
 } catch (Throwable t) {
+LOG.info("Async loop Exception Stacktrace is: {} ", 
getStackTrace(t));
--- End diff --

The method can return at line 370 without reaching 372. We had instances 
where users could not understand root cause of the interrupt exception.


---

[GitHub] storm pull request #2630: STORM-3024: Allow for scheduling to happen in the ...

[revans2]

Github user revans2 closed the pull request at:

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


---

[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...

[arunmahadevan]

GitHub user arunmahadevan opened a pull request:

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

STORM-3035: fix the issue in JmsSpout.ack when toCommit is empty



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

$ git pull https://github.com/arunmahadevan/storm STORM-3035

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

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


commit f091f0ffda8bf6b1fc52981f38cca757d9c98559
Author: Arun Mahadevan 
Date:   2018-04-19T17:52:38Z

STORM-3035: fix the issue in JmsSpout.ack when toCommit is empty




---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[srdo]

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

https://github.com/apache/storm/pull/2638#discussion_r182833894
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -361,6 +363,7 @@ public void run() {
 Time.sleep(s);
 }
 } catch (Throwable t) {
+LOG.info("Async loop Exception Stacktrace is: {} ", 
getStackTrace(t));
--- End diff --

Why isn't the stack trace available from the log in line 372? 


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[srdo]

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

https://github.com/apache/storm/pull/2638#discussion_r182833344
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -109,6 +109,8 @@
 
 import javax.security.auth.Subject;
 
+import static org.apache.commons.lang.exception.ExceptionUtils.*;
--- End diff --

+1. A little odd that checkstyle didn't catch this, since it's in the rules 
https://github.com/apache/storm/blob/master/storm-checkstyle/src/main/resources/storm/storm_checkstyle.xml#L82.
 @kishorvpatil could you check if maybe the number of allowed style violations 
for storm-client can be reduced?


---

[GitHub] storm issue #2621: [STORM-3017] Refactor pacemaker client exception handling

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2621
  
+1, thanks for fixing this.


---

[GitHub] storm pull request #2627: STORM-3022 Decouple storm-hive UTs with Hive (1.x)

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[Ethanlm]

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

https://github.com/apache/storm/pull/2638#discussion_r182765656
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -109,6 +109,8 @@
 
 import javax.security.auth.Subject;
 
+import static org.apache.commons.lang.exception.ExceptionUtils.*;
--- End diff --

nit: don't use *


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[kishorvpatil]

GitHub user kishorvpatil opened a pull request:

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

[STORM-3034] Adding exception stacktrace for executor failures in worker



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

$ git pull https://github.com/kishorvpatil/incubator-storm storm3034

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

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


commit c289c47c98307e20bdacbfe2f26d4655963c392f
Author: Kishor Patil 
Date:   2018-04-19T14:01:27Z

Adding exception stacktrace for executor failures in worker




---

[GitHub] storm pull request #2631: [STORM-3025] Optimize Cluster methods with Caching...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] storm pull request #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_W...

[Ethanlm]

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

https://github.com/apache/storm/pull/2634#discussion_r182758538
  
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/Worker.java 
---
@@ -305,8 +305,9 @@ private void setupFlushTupleTimer(final Map topologyConf, final
 }
 
 private void setupBackPressureCheckTimer(final Map 
topologyConf) {
-final Integer workerCount = 
ObjectReader.getInt(topologyConf.get(Config.TOPOLOGY_WORKERS));
-if (workerCount <= 1) {
+Set nonLocalTasks = 
Sets.difference(workerState.getTaskToComponent().keySet(),
--- End diff --


https://github.com/apache/storm/blob/master/examples/storm-loadgen/src/main/java/org/apache/storm/loadgen/CaptureLoad.java#L109
 This is fine because the code also gets the num of workers from topologyInfo.


https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/daemon/StormCommon.java#L363-L364
 this is also fine because we override the default value of 
`TOPOLOGY_EVENTLOGGER_EXECUTORS` when we submit the topology if it's on RAS.
Similar to 
https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/daemon/StormCommon.java#L276





---

[GitHub] storm pull request #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_W...

[Ethanlm]

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

https://github.com/apache/storm/pull/2634#discussion_r182757546
  
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/Worker.java 
---
@@ -305,8 +305,9 @@ private void setupFlushTupleTimer(final Map topologyConf, final
 }
 
 private void setupBackPressureCheckTimer(final Map 
topologyConf) {
-final Integer workerCount = 
ObjectReader.getInt(topologyConf.get(Config.TOPOLOGY_WORKERS));
-if (workerCount <= 1) {
+Set nonLocalTasks = 
Sets.difference(workerState.getTaskToComponent().keySet(),
--- End diff --

About the `set difference calls`, I thought these two are different. The 
first one 
https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/daemon/worker/Worker.java#L230
 is to check if there is any outbound task not in local worker. This doesn't 
necessarily mean the topology has only one worker. 
The second (in setupBackPressureCheckTimer) is to check if all the tasks 
are in local which means there is only one worker.





---

[GitHub] storm pull request #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_W...

[Ethanlm]

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

https://github.com/apache/storm/pull/2634#discussion_r182755817
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -3032,9 +3019,19 @@ public void submitTopologyWithOpts(String topoName, 
String uploadedJarLocation,
 // if the other config does not have it set.
 topology = normalizeTopology(totalConf, topology);
 
-//set the number of acker executors;
-totalConfToSave.put(Config.TOPOLOGY_ACKER_EXECUTORS, 
getNumOfAckerExecs(totalConf, topology));
-LOG.debug("Config.TOPOLOGY_ACKER_EXECUTORS set to: {}", 
totalConfToSave.get(Config.TOPOLOGY_ACKER_EXECUTORS));
+// if the Resource Aware Scheduler is used,
+// we might need to set the number of acker executors and 
eventlogger executors to be the estimated number of workers.
+if (ServerUtils.isRAS(conf)) {
+int estimatedNumWorker = 
ServerUtils.getEstimatedWorkerCountForRASTopo(totalConf, topology);
+int numAckerExecs = 
ObjectReader.getInt(totalConf.get(Config.TOPOLOGY_ACKER_EXECUTORS), 
estimatedNumWorker);
+int numEventLoggerExecs = 
ObjectReader.getInt(totalConf.get(Config.TOPOLOGY_EVENTLOGGER_EXECUTORS), 
estimatedNumWorker);
+
+totalConfToSave.put(Config.TOPOLOGY_ACKER_EXECUTORS, 
numAckerExecs);
--- End diff --

+1


---

[GitHub] storm pull request #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_W...

[revans2]

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

https://github.com/apache/storm/pull/2634#discussion_r182750384
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -3032,9 +3019,19 @@ public void submitTopologyWithOpts(String topoName, 
String uploadedJarLocation,
 // if the other config does not have it set.
 topology = normalizeTopology(totalConf, topology);
 
-//set the number of acker executors;
-totalConfToSave.put(Config.TOPOLOGY_ACKER_EXECUTORS, 
getNumOfAckerExecs(totalConf, topology));
-LOG.debug("Config.TOPOLOGY_ACKER_EXECUTORS set to: {}", 
totalConfToSave.get(Config.TOPOLOGY_ACKER_EXECUTORS));
+// if the Resource Aware Scheduler is used,
+// we might need to set the number of acker executors and 
eventlogger executors to be the estimated number of workers.
+if (ServerUtils.isRAS(conf)) {
+int estimatedNumWorker = 
ServerUtils.getEstimatedWorkerCountForRASTopo(totalConf, topology);
+int numAckerExecs = 
ObjectReader.getInt(totalConf.get(Config.TOPOLOGY_ACKER_EXECUTORS), 
estimatedNumWorker);
+int numEventLoggerExecs = 
ObjectReader.getInt(totalConf.get(Config.TOPOLOGY_EVENTLOGGER_EXECUTORS), 
estimatedNumWorker);
+
+totalConfToSave.put(Config.TOPOLOGY_ACKER_EXECUTORS, 
numAckerExecs);
--- End diff --

No we are not overriding the user setting.  We are overriding the default 
value.


---

[GitHub] storm pull request #2634: [STORM-3021] Fix wrong usages of Config.TOPOLOGY_W...

[revans2]

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

https://github.com/apache/storm/pull/2634#discussion_r182747281
  
--- Diff: docs/Resource_Aware_Scheduler_overview.md ---
@@ -184,6 +184,10 @@ The user can set some default configurations for the 
Resource Aware Scheduler in
 topology.worker.max.heap.size.mb: 768.0
 ```
 
+### Warning
+
+The number of workers will be dynamically calculated by the Resource Aware 
Scheduler. The `Config.TOPOLOGY_WORKERS` will not be honored. 
--- End diff --

+1 for the comment from @roshannaik 


---

[GitHub] storm pull request #2635: [STORM-3029] don't use keytab based login for hbas...

[revans2]

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

https://github.com/apache/storm/pull/2635#discussion_r182745958
  
--- Diff: 
external/storm-autocreds/src/main/java/org/apache/storm/hbase/security/HBaseSecurityUtil.java
 ---
@@ -52,24 +54,27 @@ private HBaseSecurityUtil() {
 
 public static UserProvider login(Map conf, 
Configuration hbaseConfig) throws IOException {
 //Allowing keytab based login for backward compatibility.
-if (UserGroupInformation.isSecurityEnabled() && 
(conf.get(TOPOLOGY_AUTO_CREDENTIALS) == null ||
-!(((List) 
conf.get(TOPOLOGY_AUTO_CREDENTIALS)).contains(AutoHBase.class.getName() {
-LOG.info("Logging in using keytab as AutoHBase is not 
specified for " + TOPOLOGY_AUTO_CREDENTIALS);
-//insure that if keytab is used only one login per process 
executed
-if(legacyProvider == null) {
-synchronized (HBaseSecurityUtil.class) {
-if(legacyProvider == null) {
-legacyProvider = 
UserProvider.instantiate(hbaseConfig);
-String keytab = (String) 
conf.get(STORM_KEYTAB_FILE_KEY);
-if (keytab != null) {
-hbaseConfig.set(STORM_KEYTAB_FILE_KEY, keytab);
-}
-String userName = (String) 
conf.get(STORM_USER_NAME_KEY);
-if (userName != null) {
-hbaseConfig.set(STORM_USER_NAME_KEY, userName);
+if (UserGroupInformation.isSecurityEnabled()) {
+List autoCredentials = (List) 
conf.get(TOPOLOGY_AUTO_CREDENTIALS);
+if ((autoCredentials == null)
+|| 
(!autoCredentials.contains(AutoHBase.class.getName()) && 
!autoCredentials.contains(AutoTGT.class.getName( {
+LOG.info("Logging in using keytab as either AutoHBase or 
AutoTGT is specified for " + TOPOLOGY_AUTO_CREDENTIALS);
--- End diff --

nit: I think it should be neither instead of either


---

[GitHub] storm issue #2637: Map of Spout configurations from `storm-kafka` to `storm-...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2637
  
I read the documentation for maxOffsetBehind you linked, and I think it is 
confusingly phrased. It says 

> If a failing tuple's offset is less than maxOffsetBehind, the spout stops 
retrying the tuple

and what they mean is 

> If a failing tuple's offset is farther behind the current offset than 
maxOffsetBehind (i.e. if `currentOffset - failingOffset > maxOffsetBehind`, the 
spout stops retrying the tuple

I misread the description as meaning

> If a failing tuple's offset is less than maxOffsetBehind behind the 
current offset (i.e. if `currentOffset - failingOffset < maxOffsetBehind`), the 
spout stops retrying the tuple

The links are pointing into the javadocs directory, which is not checked 
into git. The javadocs directory is supposed to be created when someone 
publishes release documentation to the site. See the documentation at 
https://github.com/apache/storm-site#adding-a-new-release-to-the-website for 
how to generate the javadoc.

I'd expect the links to work if you check out the repo and generate the 
javadoc.


---