[GitHub] storm issue #2916: STORM-3292: flush writers in HiveState when the trident b...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2916 @HeartSaVioR its mainly the tick tuple interval in HiveOptions that does not apply to trident. For now may be we can log some warnings if that is set for trident. Refactoring the code can be taken up as a followup. We will have to first deprecate the existing apis/constructor in HiveState that accepts Hive options and then provide an alternate Options for Trident. ---
[GitHub] storm pull request #2916: STORM-3292: flush writers in HiveState when the tr...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2916 STORM-3292: flush writers in HiveState when the trident batch commits You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3292 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2916.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 #2916 commit 07634fcb881a53d548827f27b1a9388ff8982f5e Author: Arun Mahadevan Date: 2018-11-27T02:15:58Z STORM-3292: flush writers in HiveState when the trident batch commits ---
[GitHub] storm pull request #2909: STORM-3123 - add support for Kafka security config...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2909 STORM-3123 - add support for Kafka security config in storm-kafka-monitor 1.x version of https://github.com/apache/storm/pull/2906 You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3123-1.x Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2909.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 #2909 commit 371cc269a6dec5ee6172d7631f7f5574d6eab566 Author: Vipin Rathor Date: 2018-07-12T00:01:36Z STORM-3123 - add support for Kafka security config in storm-kafka-monitor commit bbe7827987dc03393652740522975b7bf0169c64 Author: Arun Mahadevan Date: 2018-11-12T19:19:31Z STORM-3123: Changes to return extra properties from KafkaSpout and use it in TopologySpoutLag commit 3e655cba0a621aae24a3300c2b75d1b15300032b Author: Arun Mahadevan Date: 2018-11-16T01:12:44Z STORM-3123: Address review comments ---
[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...
Github user arunmahadevan commented on a diff in the pull request: https://github.com/apache/storm/pull/2906#discussion_r233963942 --- Diff: external/storm-kafka-monitor/src/main/java/org/apache/storm/kafka/monitor/NewKafkaSpoutOffsetQuery.java --- @@ -27,12 +27,15 @@ private final String consumerGroupId; // consumer group id for which the offset needs to be calculated private final String bootStrapBrokers; // bootstrap brokers private final String securityProtocol; // security protocol to connect to kafka +private final String consumerConfig; // security configuration file to connect to secure kafka --- End diff -- it can be any properties. renamed to `consumerPropertiesFileName`. ---
[GitHub] storm issue #2906: STORM-3123 - add support for Kafka security config in sto...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2906 @HeartSaVioR thanks for reviewing. Addressed comments. ---
[GitHub] storm issue #2906: STORM-3123 - add support for Kafka security config in sto...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2906 Have tested the changes with Kafka broker running in 2-way SSL and unsecure modes and able to see the lags. The change on the kafka spout side is to return the KafkaConfig properties in getComponentConfiguration so that the lag util can use it. ---
[GitHub] storm issue #2906: STORM-3123 - add support for Kafka security config in sto...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2906 @VipinRathor I pulled in the relevant changes from https://github.com/apache/storm/pull/2760 and created this so that we can take it forward. I need to do some tests and will update once done. cc @priyank5485 @HeartSaVioR ---
[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2906 STORM-3123 - add support for Kafka security config in storm-kafka-monitor You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3123 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2906.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 #2906 commit b42ff565ea7ed17c309912ed95425c14c62b3a36 Author: Vipin Rathor Date: 2018-07-12T00:01:36Z STORM-3123 - add support for Kafka security config in storm-kafka-monitor commit 3e30ab2954e4ebb62dd91b91247eb11dbbbff78a Author: Arun Mahadevan Date: 2018-11-12T19:19:31Z STORM-3123: Changes to return extra properties from KafkaSpout and use it in TopologySpoutLag Change-Id: I69e55abbb9c0e84cfd2b2f5fcd07d1ab6ef19dc4 ---
[GitHub] storm issue #2871: [STORM-3252] Bug fix for blobstore sync
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2871 @revans2 , updated the log message, please check if it makes sense. @kishorvpatil I am not sure if we want to swallow IOException, since it may be due to some serious problem which we want to propagate and nimbus cannot continue in that case. ---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224559083
--- Diff:
storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map conf, BlobStore bl
out.close();
}
isSuccess = true;
+} catch(FileNotFoundException fnf) {
+LOG.warn("Blobstore file for key '%s' does not exist or
got deleted before it could be downloaded.", key, fnf);
--- End diff --
Thanks for catching.
---
[GitHub] storm issue #2773: Blobstore sync bug fix
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2773 @HeartSaVioR created https://github.com/apache/storm/pull/2871 and linked to a JIRA. Maybe good to get this in 2.0 release. ---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224541248
--- Diff:
storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map conf, BlobStore bl
out.close();
}
isSuccess = true;
+} catch(FileNotFoundException fnf) {
+LOG.warn("FileNotFoundException", fnf);
--- End diff --
It crashes the nimbus if the exception is propagated. (we have seen this
happening if topology gets killed while the non-leader nimbus tries to download
the blob). It may be better to swallow and let `downloadUpdatedBlob` return
false here?
---
[GitHub] storm issue #2871: [STORM-3252] Bug fix for blobstore sync
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2871 Cherry picked commits from https://github.com/apache/storm/pull/2773 and linked with a JIRA. ---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2871 [STORM-3252] Bug fix for blobstore sync 1.Bug fix for blob sync frequency with time unit error. 2.Bug fix for blob sync delete file, add catch NoSuchFileException. 3.Bug fix for blob sync update blob flie, add catch FileNotFoundException You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3252 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2871.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 #2871 commit 94e6bbcc873c04034c55977653910b9921bc014c Author: jiangzhileaf Date: 2018-07-19T08:37:22Z Bug fix for blobstore sync. 1.Bug fix for blob sync frequency with time unit error. 2.Bug fix for blob sync delete file, add catch NoSuchFileException. 3.Bug fix for blob sync update blob flie, add catch FileNotFoundException ---
[GitHub] storm issue #2829: STORM-3222: Fix KafkaSpout internals to use LinkedList in...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2829 @revans2 thanks for merging. Raised https://github.com/apache/storm/pull/2835 for 1.x-branch. >If you want to send more it can be more efficient, but you risk going over the max.spout.pending amount that is set by the user, and in 2.x where an emit can block, you risk having the spout block as well. If the emit can block in 2.x it might be an issue to address because its not in spout contract to necessarily emit only one record. I thought it could overflow and the max.spout.pending is not that relevant with the back pressure changes. ---
[GitHub] storm pull request #2835: STORM-3222: Fix KafkaSpout internals to use Linked...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2835 STORM-3222: Fix KafkaSpout internals to use LinkedList instead of ArrayList KafkaSpout internally maintains a waitingToEmit list per topic partition and keeps removing the first item to emit during each nextTuple. The implementation uses an ArrayList which results in un-necessary traversal and copy for each tuple. Also I am not sure why the nextTuple only emits a single tuple wheres ideally it should emit whatever it can emit in a single nextTuple call which is more efficient. However the logic appears too complicated to refactor. https://github.com/apache/storm/pull/2829 for 1.x You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3222-1.x Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2835.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 #2835 commit 2e0a01e5e4e36169ef2a45bf5dcd72792231ee07 Author: Arun Mahadevan Date: 2018-09-12T22:36:24Z STORM-3222: Fix KafkaSpout internals to use LinkedList instead of ArrayList KafkaSpout internally maintains a waitingToEmit list per topic partition and keeps removing the first item to emit during each nextTuple. The implementation uses an ArrayList which results in un-necessary traversal and copy for each tuple. Also I am not sure why the nextTuple only emits a single tuple wheres ideally it should emit whatever it can emit in a single nextTuple call which is more efficient. However the logic appears too complicated to refactor. ---
[GitHub] storm issue #2829: STORM-3222: Fix KafkaSpout internals to use LinkedList in...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2829 >what's the motivation to change this to LinkedList? Its mentioned in the description. Heres the relevant code for some more details - https://github.com/apache/storm/blob/master/external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java#L422 >nextTuple emits only a single tuple because that's the contract of the method nextTuple, which must be honored. This was thoroughly discussed in the patch with the initial code implementation. It can emit one or more and ideally should emit whatever it has to emit at that point. https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/spout/ISpout.java#L72 ---
[GitHub] storm issue #2829: STORM-3222: Fix KafkaSpout internals to use LinkedList in...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2829 Also I am not sure why the nextTuple only emits a single tuple wheres ideally it should emit whatever it can emit in a single nextTuple call which is more efficient. However the logic appears too complicated to refactor. @srdo @HeartSaVioR ---
[GitHub] storm pull request #2829: STORM-3222: Fix KafkaSpout internals to use Linked...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2829 STORM-3222: Fix KafkaSpout internals to use LinkedList instead of ArrayList KafkaSpout internally maintains a waitingToEmit list per topic partition and keeps removing the first item to emit during each nextTuple. The implementation uses an ArrayList which results in un-necessary traversal and copy for each tuple. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3222 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2829.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 #2829 commit a5384aa845496a7e584bd947cace968a18b7ffdf Author: Arun Mahadevan Date: 2018-09-12T22:36:24Z STORM-3222: Fix KafkaSpout internals to use LinkedList instead of ArrayList KafkaSpout internally maintains a waitingToEmit list per topic partition and keeps removing the first item to emit during each nextTuple. The implementation uses an ArrayList which results in un-necessary traversal and copy for each tuple. Also I am not sure why the nextTuple only emits a single tuple wheres ideally it should emit whatever it can emit in a single nextTuple call which is more efficient. However the logic appears too complicated to refactor. ---
[GitHub] storm pull request #2498: STORM-2884: Explicitly specify the curator depende...
Github user arunmahadevan closed the pull request at: https://github.com/apache/storm/pull/2498 ---
[GitHub] storm issue #2773: Blobstore sync bug fix
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2773 Seems to fix a few race conditions, may be we can take this forward. ---
[GitHub] storm pull request #2773: Blobstore sync bug fix
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2773#discussion_r216796216
--- Diff:
storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map conf, BlobStore bl
out.close();
}
isSuccess = true;
+} catch(FileNotFoundException fnf) {
--- End diff --
Btw, theres a race condition where the topology gets killed while the blob
download is going on and apparently it throws a FNF exception here when
invoking out.close().
---
[GitHub] storm issue #2811: STORM-3184: Replace the usage of redact-value with Config...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2811 cc @HeartSaVioR ---
[GitHub] storm pull request #2811: STORM-3184: Replace the usage of redact-value with...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2811 STORM-3184: Replace the usage of redact-value with ConfigUtils.maskPasswords The topology submission can fail since redact-value expects a clojure map. We don't need redact-value, it can be replaced with ConfigUtils.maskPasswords. This is already done in master. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3184-followup Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2811.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 #2811 commit 43faecc870468a00d5f1b2d5ba1c5c4274ae77b0 Author: Arun Mahadevan Date: 2018-08-20T23:31:32Z STORM-3184: Replace the usage of redact-value with ConfigUtils.maskPasswords The topology submission can fail since redact-value expects a clojure map. We dont need redact-value, it can be replaced with just ConfigUtils.maskPasswords. This is already done in master. ---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209442754
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+public static Map maskPasswords(final Map conf) {
+Maps.EntryTransformer maskPasswords =
--- End diff --
@HeartSaVioR , thanks for the suggestion, we can replace
STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD too.
I was not aware of the "shaded-deps" module and its usage.
Reverted to the earlier approach and using the shaded-deps now.
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209432875
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+public static Map maskPasswords(final Map conf) {
+Maps.EntryTransformer maskPasswords =
--- End diff --
The problem is that the map is traversed and copied even if we don't intend
to log the conf and I was trying to avoid it using the Guava transformer. It
may not be a major issue now since the conf is logged only once at the
beginning, but wanted to avoid any potential future misuse.
I could not figure out a way to do the same with java 8 Streams API. I have
refactored it to return a wrapped object which will be evaluated only when its
required to log. Please take a look and let me know if it makes sense or if
theres a better way.
---
[GitHub] storm issue #2798: STORM-3184: Mask the plaintext passwords from the logs
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2798 @HeartSaVioR heres the master port - https://github.com/apache/storm/pull/2801 I guess sooner we release 2.0 and stick to master branch for even minor feature development the better. ---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2801 STORM-3184: Mask the plaintext passwords from the logs Introduce a `Password` config annotation and use it to mark configs that are sensitive and mask the values while logging. Master port of https://github.com/apache/storm/pull/2798 You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3184-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2801.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 #2801 commit 9d36724e88f54ae48fd892b001eeb76313adc9da Author: Arun Mahadevan Date: 2018-08-11T00:39:49Z STORM-3184: Mask the plaintext passwords from the logs Introduce a `Password` config annotation and use it to mark configs that are sensitive and mask the values while logging. ---
[GitHub] storm issue #2798: STORM-3184: Mask the plaintext passwords from the logs
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2798 @HeartSaVioR , i had a quick look and its not straightforward to port this patch to master. The Configs are now split into Config and DaemonConfig and quite a lot of refactoring has happened. I need to figure out the right place to put this so that the code can be shared across the storm-client and storm-server modules. ---
[GitHub] storm issue #2798: STORM-3184: Mask the plaintext passwords from the logs
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2798 ping @HeartSaVioR ---
[GitHub] storm pull request #2798: STORM-3184: Mask the plaintext passwords from the ...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2798 STORM-3184: Mask the plaintext passwords from the logs Introduce a `Password` config annotation and use it to mark configs that are sensitive and mask the values while logging. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3184 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2798.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 #2798 commit fc942ee10d86649db9e9b8ce3dc0a04ea23439ce Author: Arun Mahadevan Date: 2018-08-07T19:13:54Z STORM-3184: Mask the plaintext passwords from the logs Introduce a `Password` config annotation and use it to mark configs that are sensitive and mask the values while logging. ---
[GitHub] storm issue #2777: (1.x) STORM-3161 Local mode should force setting min repl...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2777 +1 ---
[GitHub] storm issue #2776: STORM-3161 Local mode should force setting min replicatio...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2776 +1 ---
[GitHub] storm issue #2737: (1.x) STORM-3122 Avoid supervisor being crashed due to ra...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2737 +1, LGTM ---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2721 @revans2 , i have refactored the code so that we check for "user.name" in the normal container and the result of `getWorkerUser` in the RunAsUserContainer. The `getWorkerUser` is used in other places to set the user in the workers local state and apparently used to do some authorization checks in the log viewer. I did not want to affect that behavior so I introduced a new function to return the OS user that the worker is expected to run as that invokes the `getWorkerUser` in the run as container. This issue happens in posix environment as well and most of the users dont set the `supervisor.run.worker.as.user` config but run storm in secure mode. ---
[GitHub] storm issue #2691: STORM-3061: Update version of hbase
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2691 +1, Looks good. ---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2721 Also note that supervisor might be killing the workers in response to an assignment change triggered by nimbus (may be due to user triggered kill or rebalance) and the supervisor has no idea of which end user/action might have triggered this. The authorization checks to see if the user has permissions to kill/rebalance the topology is already happening at nimbus so I think we are good. ---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2721 Unless `supervisor.run.worker.as.user` is set, the worker process runs as "storm" user. I guess the supervisor should always check "if all processes are dead" by just looking if the worker pids are alive than doing user comparison, since there is no mapping between the user that launched the topology (e.g kerberos user) and the actual OS user that worker is running as (this is always storm).. In the "run as user" container the "kill" command is launched by switching to the OS user that worker is actually running as here - https://github.com/apache/storm/blob/1.x-branch/storm-core/src/jvm/org/apache/storm/daemon/supervisor/RunAsUserContainer.java#L55 and that should take care of the security. ---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2721 @revans2 , @HeartSaVioR, can you take a look? ---
[GitHub] storm pull request #2721: STORM-3110: Skip the user while checking isProcess...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2721 STORM-3110: Skip the user while checking isProcessAlive You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-3110-1 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2721.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 #2721 commit 1d43937a12d5632d1c64e5701b8443a179595062 Author: Arun Mahadevan Date: 2018-06-15T23:32:53Z STORM-3110: Skip the user while checking isProcessAlive ---
[GitHub] storm issue #2712: Update Multilang-protocol.md
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2712 Looks good. ---
[GitHub] storm issue #2691: STORM-3061: Update version of hbase
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2691 Actually I was wrong. storm-hdfs, storm-hbase, storm-hive seem to depend on storm-autocreds. I guess we could pull out the required classes into some common package as part of the follow up JIRA. ---
[GitHub] storm issue #2691: STORM-3061: Update version of hbase
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2691 Sure we can revisit this in a follow up JIRA. We may not have to split the autocreds since none of the other components depends on it. The hbase-server dependency if included is just going to end up under external/storm-autocreds and not going to be included in the class path by default. We could also check with the Hbase team to pull out the TokenUtils into hbase-client package. ---
[GitHub] storm issue #2692: STORM-3083 Upgrade HikariCP version to 2.4.7
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2692 +1 ---
[GitHub] storm pull request #2691: STORM-3061: Update version of hbase
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2691#discussion_r190424484
--- Diff: pom.xml ---
@@ -294,7 +294,7 @@
0.14.0
2.6.1
${hadoop.version}
-1.1.12
+1.4.4
--- End diff --
Since we are upgrading anyway, do we want to attempt Hbase 2.0 ?
---
[GitHub] storm issue #2687: STORM-3061: thrift 0.11
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2687 +1 LGTM. ---
[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2639 @ptgoetz , can you take a look again? ---
[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2639 @ptgoetz , thanks for the suggestion. Yes, it was clear why the distributed flag was needed when it was not used anywhere else in the code. I was hoping that the user will set the right parallelism while consuming from topics. I can add it back. ---
[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2639 @HeartSaVioR , I made some changes so that the order of the methods does not matter and the final validation happens in "open". I also ran the example topology and things look fine. ---
[GitHub] storm issue #2672: (1.x-branch) STORM-3069 Allow users to specify maven loca...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2672 +1 LGTM ---
[GitHub] storm issue #2671: STORM-3069 Allow users to specify maven local repository ...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2671 +1 LGTM. ---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request: https://github.com/apache/storm/pull/2639#discussion_r187500706 --- Diff: external/storm-jms/pom.xml --- @@ -94,7 +94,7 @@ maven-checkstyle-plugin -63 +73 --- End diff -- I cant figure out why checkstyle keeps complaining. Running "mvn checkstyle:check" doesnt throw any warnings for JMSSpout.java. I have set it to 64 for the build to pass. And the rules we have seems too restrictive. (Java-doc for each variable/method and line length of 80). Should probably relook so that its not a time waste. ---
[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2639 @HeartSaVioR , please check it again. ---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2639#discussion_r187496374
--- Diff:
external/storm-jms/src/main/java/org/apache/storm/jms/spout/JmsSpout.java ---
@@ -403,50 +274,31 @@ public void ack(Object msgId) {
* Will only be called if we're transactional or not
AUTO_ACKNOWLEDGE
*/
@Override
-public void fail(Object msgId) {
+public void fail(final Object msgId) {
LOG.warn("Message failed: " + msgId);
-this.pendingMessages.clear();
-this.toCommit.clear();
-synchronized (this.recoveryMutex) {
-this.hasFailures = true;
-}
+messageHandler.fail(msgId);
}
/**
- * Use the {@link #tupleProducer} to determine which fields are about
to be emitted.
+ * Use the {@link #tupleProducer} to determine which fields are about
+ * to be emitted.
*
- * Note that {@link #nextTuple()} always emits to the default
stream, and thus only fields declared
- * for this stream are used.
+ * Note that {@link #nextTuple()} always emits to the default
stream,
+ * and thus only fields declared for this stream are used.
*/
@Override
-public void declareOutputFields(OutputFieldsDeclarer declarer) {
+public void declareOutputFields(final OutputFieldsDeclarer declarer) {
this.tupleProducer.declareOutputFields(declarer);
}
/**
- * Returns true if the spout has received failures from
which it has not yet recovered.
- *
- * @return {@code true} if there were failures, {@code false}
otherwise.
- */
-public boolean hasFailures() {
-return this.hasFailures;
-}
-
-/**
- * Marks a healthy session state.
- */
-protected void recovered() {
-this.hasFailures = false;
-}
-
-/**
- * Sets the periodicity of the timer task that checks for failures and
recovers the JMS session.
+ * Sets the periodicity of the timer task that
+ * checks for failures and recovers the JMS session.
*
* @param period desired wait period
*/
-public void setRecoveryPeriodMs(long period) {
-this.recoveryPeriodMs = period;
+public void setRecoveryPeriodMs(final long period) {
--- End diff --
I just left it since its breaking the public API and if someone is using
this in their code.
---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2639#discussion_r187496258
--- Diff:
external/storm-jms/src/main/java/org/apache/storm/jms/spout/JmsSpout.java ---
@@ -262,42 +189,26 @@ public void onMessage(Message msg) {
* topic/queue.
*/
@Override
-public void open(Map<String, Object> conf,
- TopologyContext context,
- SpoutOutputCollector collector) {
+public void open(final Map<String, Object> conf,
+ final TopologyContext context,
+ final SpoutOutputCollector spoutOutputCollector) {
-if (this.jmsProvider == null) {
-throw new IllegalStateException("JMS provider has not been
set.");
-}
-if (this.tupleProducer == null) {
-throw new IllegalStateException("JMS Tuple Producer has not
been set.");
+if (jmsProvider == null) {
+throw new IllegalStateException(
+"JMS provider has not been set.");
}
-// TODO get the default value from storm instead of hard coding 30
secs
-Long topologyTimeout =
-((Number)
conf.getOrDefault(Config.TOPOLOGY_MESSAGE_TIMEOUT_SECS,
DEFAULT_MESSAGE_TIMEOUT_SECS)).longValue();
-if ((TimeUnit.SECONDS.toMillis(topologyTimeout)) >
this.recoveryPeriodMs) {
-LOG.warn("*** WARNING *** : "
- + "Recovery period (" + this.recoveryPeriodMs + "
ms.) is less then the configured "
- + "'topology.message.timeout.secs' of " +
topologyTimeout
- + " secs. This could lead to a message replay
flood!");
+if (tupleProducer == null) {
+throw new IllegalStateException(
+"JMS Tuple Producer has not been set.");
}
-this.queue = new LinkedBlockingQueue();
-this.toCommit = new TreeSet();
-this.pendingMessages = new HashMap<JmsMessageID, Message>();
-this.collector = collector;
+collector = spoutOutputCollector;
try {
-ConnectionFactory cf = this.jmsProvider.connectionFactory();
-Destination dest = this.jmsProvider.destination();
-this.connection = cf.createConnection();
-this.session = connection.createSession(false,
this.jmsAcknowledgeMode);
-MessageConsumer consumer = session.createConsumer(dest);
-consumer.setMessageListener(this);
-this.connection.start();
-if (this.isDurableSubscription() && this.recoveryPeriodMs > 0)
{
-this.recoveryTimer = new Timer();
-this.recoveryTimer.scheduleAtFixedRate(new RecoveryTask(),
RECOVERY_DELAY_MS, this.recoveryPeriodMs);
-}
-
+ConnectionFactory cf = jmsProvider.connectionFactory();
+Destination dest = jmsProvider.destination();
+connection = cf.createConnection();
+session = messageHandler.createSession(connection);
--- End diff --
We can throw an exception if `setIndividualAck` is invoked and the ACK mode
is still the standard ones.
---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2639#discussion_r187496121
--- Diff:
external/storm-jms/src/main/java/org/apache/storm/jms/spout/JmsSpout.java ---
@@ -18,164 +18,124 @@
package org.apache.storm.jms.spout;
-import java.io.Serializable;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Timer;
-import java.util.TimerTask;
-import java.util.TreeSet;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.TimeUnit;
-import javax.jms.Connection;
-import javax.jms.ConnectionFactory;
-import javax.jms.Destination;
-import javax.jms.JMSException;
-import javax.jms.Message;
-import javax.jms.MessageConsumer;
-import javax.jms.MessageListener;
-import javax.jms.Session;
-import org.apache.storm.Config;
import org.apache.storm.jms.JmsProvider;
import org.apache.storm.jms.JmsTupleProducer;
import org.apache.storm.spout.SpoutOutputCollector;
import org.apache.storm.task.TopologyContext;
import org.apache.storm.topology.OutputFieldsDeclarer;
import org.apache.storm.topology.base.BaseRichSpout;
import org.apache.storm.tuple.Values;
-import org.apache.storm.utils.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.Destination;
+import javax.jms.JMSException;
+import javax.jms.Message;
+import javax.jms.MessageConsumer;
+import javax.jms.Session;
+import java.util.HashMap;
+import java.util.Map;
+
/**
- * A Storm Spout implementation that listens to a JMS topic
or queue and outputs tuples based on the messages it receives.
+ * A Storm Spout implementation that listens to a JMS topic or
+ * queue and outputs tuples based on the messages it receives.
*
* JmsSpout instances rely on JmsProducer
* implementations to obtain the JMS
* ConnectionFactory and Destination objects
necessary
* to connect to a JMS topic/queue.
*
- * When a JmsSpout receives a JMS message, it delegates to
an
- * internal JmsTupleProducer instance to create a Storm tuple
from the incoming message.
+ * When a {@code JmsSpout} receives a JMS message, it delegates to an
+ * internal {@code JmsTupleProducer} instance to create a Storm tuple from
+ * the incoming message.
*
* Typically, developers will supply a custom
JmsTupleProducer
* implementation appropriate for the expected message content.
*/
@SuppressWarnings("serial")
-public class JmsSpout extends BaseRichSpout implements MessageListener {
+public class JmsSpout extends BaseRichSpout {
-/**
- * The logger object instance for this class.
- */
+/** The logger object instance for this class. */
private static final Logger LOG =
LoggerFactory.getLogger(JmsSpout.class);
-/**
- * The logger of the recovery task.
- */
-private static final Logger RECOVERY_TASK_LOG =
LoggerFactory.getLogger(RecoveryTask.class);
-
-/**
- * Time to sleep between queue polling attempts.
- */
+/** Time to sleep between queue polling attempts. */
private static final int POLL_INTERVAL_MS = 50;
-/**
- * The default value for {@link Config#TOPOLOGY_MESSAGE_TIMEOUT_SECS}.
- */
-private static final int DEFAULT_MESSAGE_TIMEOUT_SECS = 30;
-
-/**
- * Time to wait before queuing the first recovery task.
- */
-private static final int RECOVERY_DELAY_MS = 10;
-/**
- * Used to safely recover failed JMS sessions across instances.
- */
-private final Serializable recoveryMutex = "RECOVERY_MUTEX";
/**
* The acknowledgment mode used for this instance.
*
* @see Session
*/
private int jmsAcknowledgeMode = Session.AUTO_ACKNOWLEDGE;
-/**
- * Indicates whether or not this spout should run as a singleton.
- */
+
+/** Sets up the way we want to handle the emit, ack and fails. */
+private transient MessageHandler messageHandler = new MessageHandler();
+
+/** Indicates whether or not this spout should run as a singleton. */
private boolean distributed = true;
-/**
- * Used to generate tuples from incoming messages.
- */
+
+/** Used to generate tuples from incoming messages. */
private JmsTupleProducer tupleProducer;
-/**
- * Encapsulates jms related classes needed to communicate with the mq.
- */
+
+/** Encapsulates jms related classes needed to communicate with the
mq.
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2639#discussion_r187496062
--- Diff:
external/storm-jms/src/main/java/org/apache/storm/jms/spout/JmsSpout.java ---
@@ -18,164 +18,124 @@
package org.apache.storm.jms.spout;
-import java.io.Serializable;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Timer;
-import java.util.TimerTask;
-import java.util.TreeSet;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.TimeUnit;
-import javax.jms.Connection;
-import javax.jms.ConnectionFactory;
-import javax.jms.Destination;
-import javax.jms.JMSException;
-import javax.jms.Message;
-import javax.jms.MessageConsumer;
-import javax.jms.MessageListener;
-import javax.jms.Session;
-import org.apache.storm.Config;
import org.apache.storm.jms.JmsProvider;
import org.apache.storm.jms.JmsTupleProducer;
import org.apache.storm.spout.SpoutOutputCollector;
import org.apache.storm.task.TopologyContext;
import org.apache.storm.topology.OutputFieldsDeclarer;
import org.apache.storm.topology.base.BaseRichSpout;
import org.apache.storm.tuple.Values;
-import org.apache.storm.utils.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.Destination;
+import javax.jms.JMSException;
+import javax.jms.Message;
+import javax.jms.MessageConsumer;
+import javax.jms.Session;
+import java.util.HashMap;
+import java.util.Map;
+
/**
- * A Storm Spout implementation that listens to a JMS topic
or queue and outputs tuples based on the messages it receives.
+ * A Storm Spout implementation that listens to a JMS topic or
+ * queue and outputs tuples based on the messages it receives.
*
* JmsSpout instances rely on JmsProducer
* implementations to obtain the JMS
* ConnectionFactory and Destination objects
necessary
* to connect to a JMS topic/queue.
*
- * When a JmsSpout receives a JMS message, it delegates to
an
- * internal JmsTupleProducer instance to create a Storm tuple
from the incoming message.
+ * When a {@code JmsSpout} receives a JMS message, it delegates to an
+ * internal {@code JmsTupleProducer} instance to create a Storm tuple from
+ * the incoming message.
*
* Typically, developers will supply a custom
JmsTupleProducer
* implementation appropriate for the expected message content.
*/
@SuppressWarnings("serial")
-public class JmsSpout extends BaseRichSpout implements MessageListener {
+public class JmsSpout extends BaseRichSpout {
-/**
- * The logger object instance for this class.
- */
+/** The logger object instance for this class. */
private static final Logger LOG =
LoggerFactory.getLogger(JmsSpout.class);
-/**
- * The logger of the recovery task.
- */
-private static final Logger RECOVERY_TASK_LOG =
LoggerFactory.getLogger(RecoveryTask.class);
-
-/**
- * Time to sleep between queue polling attempts.
- */
+/** Time to sleep between queue polling attempts. */
private static final int POLL_INTERVAL_MS = 50;
-/**
- * The default value for {@link Config#TOPOLOGY_MESSAGE_TIMEOUT_SECS}.
- */
-private static final int DEFAULT_MESSAGE_TIMEOUT_SECS = 30;
-
-/**
- * Time to wait before queuing the first recovery task.
- */
-private static final int RECOVERY_DELAY_MS = 10;
-/**
- * Used to safely recover failed JMS sessions across instances.
- */
-private final Serializable recoveryMutex = "RECOVERY_MUTEX";
/**
* The acknowledgment mode used for this instance.
*
* @see Session
*/
private int jmsAcknowledgeMode = Session.AUTO_ACKNOWLEDGE;
-/**
- * Indicates whether or not this spout should run as a singleton.
- */
+
+/** Sets up the way we want to handle the emit, ack and fails. */
+private transient MessageHandler messageHandler = new MessageHandler();
--- End diff --
Good catch. Yes I should make the MessageHandler serializable.
---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request: https://github.com/apache/storm/pull/2639#discussion_r187495978 --- Diff: external/storm-jms/pom.xml --- @@ -94,7 +94,7 @@ maven-checkstyle-plugin -63 +73 --- End diff -- Actually I can see more violations in JMSSpout.java in master branch (43) vs the patch (2). I am not sure why. Will fix it anyways. ---
[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2639 This started out as a fix to handle the exceptions in "ack" when toCommit was empty. However during the review process and testing, figured out many more issues with the current approach. Also some JMS providers like Tibco supports ACK ing individual messages, which could not be handled with the existing code. The async mode of consuming the messages was also problematic to ensure at-least once delivery even with locks/synchronization since ack-ing an individual JMS message in CLIENT_ACK mode was going to ack the messages received in the listener (even if the listener did not return). To handle all the issues, I have refactored quite and bit and changed the approach of consuming the messages from async (listener based) to sync (receive) and introduced MessageHandlers to handle the emit/ack/fail in different ways based on the mode. @HeartSaVioR , can you review it again and let me know what you think? ---
[GitHub] storm issue #2664: STORM-2884: Remove storm-druid
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2664 I think if we can shade all the storm-druid dependencies we could probably leave it there. At-least a few of the other storm connectors have not been updated in a while but we still keep it. Similarly we can just mark storm-druid deprecated (and recommend kafka based ingestion) in the README.md or somewhere. By 3.0 timeline, if there are no users we can even consider dropping it. ---
[GitHub] storm issue #2664: STORM-2884: Remove storm-druid
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2664 Lets wait for a response. I think we can add some notes saying that storm-druid is deprecated and shade its dependencies than completely removing it since there are some users using it. This can be addressed as a part of STORM-2884. ---
[GitHub] storm issue #2664: STORM-2884: Remove storm-druid
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2664 @revans2 thanks for understanding the concerns and agree that we should do something about the long term support for tranquility. We can probably poke them like @srdo suggested to get out a new release with updated dependencies and see if we can make progress. Irrespective of this, it makes sense to shade common libraries (like curator) into storm client to avoid conflicts with user topology code and thanks @revans2 for offering to take this up. ---
[GitHub] storm issue #2665: STORM-2988 Error on initialization of server mk-worker
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2665 We should add docs and an example usage in the storm.yaml.example (which has graphite and console). @dbist will be great if you address this for JMXStormReporter as a part of this PR. ---
[GitHub] storm issue #2665: STORM-2988 Error on initialization of server mk-worker
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2665 @ptgoetz , any reason to use a different constant while this [config](https://github.com/apache/storm/blob/1.x-branch/storm-core/src/jvm/org/apache/storm/Config.java#L179) exists? And JmxStormReporter seem to use the constants from Config [here](https://github.com/apache/storm/blob/1.x-branch/storm-core/src/jvm/org/apache/storm/metrics2/reporters/JmxStormReporter.java#L41) and [here](https://github.com/apache/storm/blob/1.x-branch/storm-core/src/jvm/org/apache/storm/metrics2/reporters/JmxStormReporter.java#L46) ? Is there a documentation to let the users know the right key to use? ---
[GitHub] storm issue #2664: STORM-2884: Remove storm-druid
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2664 @revans2 , I don't think we should completely drop the plugin since there are users using it. There were a few options proposed https://github.com/apache/storm/pull/2498#issuecomment-355423380 and I guess we could pursue one (like shading curator within storm-druid) based on consensus. The version conflict between storm's and the connectors dependencies is going to be there in future as well so I guess this needs a more reasonable solution than dropping the connector. The druid page seem to recommend using the tranquility library due to limitations with realtime ingestion - http://druid.io/docs/latest/ingestion/stream-ingestion.html ---
[GitHub] storm pull request #2665: STORM-2988 Error on initialization of server mk-wo...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2665#discussion_r186564318
--- Diff:
storm-core/src/jvm/org/apache/storm/metrics2/reporters/JmxStormReporter.java ---
@@ -67,7 +68,7 @@ public void prepare(MetricRegistry metricsRegistry,
Map<String, Object> stormCon
}
public static String getMetricsJMXDomain(Map reporterConf) {
-return Utils.getString(reporterConf, JMX_DOMAIN);
+return
Utils.getString(reporterConf.get(Config.STORM_DAEMON_METRICS_REPORTER_PLUGIN_DOMAIN),
null);
--- End diff --
I think JMX_DOMAIN was intended to be the default value of the domain.
So may be
`Utils.getString(reporterConf.get(Config.STORM_DAEMON_METRICS_REPORTER_PLUGIN_DOMAIN),
JMX_DOMAIN)`
cc @ptgoetz
---
[GitHub] storm pull request #2665: STORM-2988 Error on initialization of server mk-wo...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2665#discussion_r186563440
--- Diff:
storm-core/src/jvm/org/apache/storm/metrics2/reporters/JmxStormReporter.java ---
@@ -67,7 +68,7 @@ public void prepare(MetricRegistry metricsRegistry,
Map<String, Object> stormCon
}
public static String getMetricsJMXDomain(Map reporterConf) {
-return Utils.getString(reporterConf, JMX_DOMAIN);
+return
Utils.getString(reporterConf.get(Config.STORM_DAEMON_METRICS_REPORTER_PLUGIN_DOMAIN),
null);
--- End diff --
I think user should be able to specify a domain so it cannot be a fixed
value like JMX_DOMAIN. So the proposed changes looks fine.
---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2639#discussion_r183539214
--- Diff:
external/storm-jms/src/main/java/org/apache/storm/jms/spout/JmsSpout.java ---
@@ -339,26 +339,26 @@ public void nextTuple() {
*/
@Override
public void ack(Object msgId) {
-
Message msg = this.pendingMessages.remove(msgId);
-JmsMessageID oldest = this.toCommit.first();
-if (msgId.equals(oldest)) {
-if (msg != null) {
-try {
-LOG.debug("Committing...");
-msg.acknowledge();
-LOG.debug("JMS Message acked: " + msgId);
-this.toCommit.remove(msgId);
-} catch (JMSException e) {
-LOG.warn("Error acknowldging JMS message: " + msgId,
e);
+if (!toCommit.isEmpty()) {
+JmsMessageID oldest = this.toCommit.first();
+if (msgId.equals(oldest)) {
+if (msg != null) {
+try {
+LOG.debug("Committing...");
+msg.acknowledge();
--- End diff --
I am not sure acking the oldest message in JMS is correct even for
`CLIENT_ACKNOWLEDGE`. This would ack the new messages that have been consumed
in the session (and possibly emitted) even before the spout received the ACK
for the message. I guess we should keep removing the message from `toCommit`
and invoke the JMS ack when its the last message in `toCommit`. (assuming we
dont consume any other message in the meanwhile).
---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2639#discussion_r183531789
--- Diff:
external/storm-jms/src/main/java/org/apache/storm/jms/spout/JmsSpout.java ---
@@ -339,26 +339,26 @@ public void nextTuple() {
*/
@Override
public void ack(Object msgId) {
-
Message msg = this.pendingMessages.remove(msgId);
-JmsMessageID oldest = this.toCommit.first();
-if (msgId.equals(oldest)) {
-if (msg != null) {
-try {
-LOG.debug("Committing...");
-msg.acknowledge();
-LOG.debug("JMS Message acked: " + msgId);
-this.toCommit.remove(msgId);
-} catch (JMSException e) {
-LOG.warn("Error acknowldging JMS message: " + msgId,
e);
+if (!toCommit.isEmpty()) {
+JmsMessageID oldest = this.toCommit.first();
+if (msgId.equals(oldest)) {
+if (msg != null) {
+try {
+LOG.debug("Committing...");
+msg.acknowledge();
--- End diff --
This piece of code was already there. I am guessing its based on the JMS
acknowledgement mode.
See - https://docs.oracle.com/cd/E19798-01/821-1841/bncfw/index.html
In `Session.CLIENT_ACKNOWLEDGE` - Acknowledging a consumed message
automatically acknowledges the receipt of all messages that have been consumed
by its session, so this logic seems fine.
The spout is ignoring Auto acknowledgement mode, but I am not sure about
the other modes like `DUPS_OK_ACKNOWLEDGE` or `SESSION_TRANSACTED` work. cc
@ptgoetz who might have more context around this.
---
[GitHub] storm issue #2643: STORM-3039 handle slot ports in TIME_WAIT state
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2643 +1 ---
[GitHub] storm pull request #2639: STORM-3035: fix the issue in JmsSpout.ack when toC...
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 <arunm@...> Date: 2018-04-19T17:52:38Z STORM-3035: fix the issue in JmsSpout.ack when toCommit is empty ---
[GitHub] storm issue #2592: STORM-2993: Storm HDFS bolt throws ClosedChannelException...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2592 @HeartSaVioR , thanks for reviewing. Raised https://github.com/apache/storm/pull/2610 for master. ---
[GitHub] storm pull request #2610: STORM-2993: Storm HDFS bolt throws ClosedChannelEx...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2610 STORM-2993: Storm HDFS bolt throws ClosedChannelException when Time rotation policy is used The TimedRotation should synchronize so that the bolt does not attempt to write to a stale writer. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2993-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2610.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 #2610 commit 85d7b73fc2c45dbf187c1f57256d796b64cf32ca Author: Arun Mahadevan <arunm@...> Date: 2018-03-12T18:53:50Z STORM-2993: Storm HDFS bolt throws ClosedChannelException when Time rotation policy is used The TimedRotation should synchronize so that the bolt does not attempt to write to a stale writer. ---
[GitHub] storm pull request #2592: STORM-2993: Storm HDFS bolt throws ClosedChannelEx...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2592 STORM-2993: Storm HDFS bolt throws ClosedChannelException when Timed rotation policy is used The TimedRotation should synchronize so that the bolt does not attempt to write to a stale writer. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2993 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2592.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 #2592 commit 9bb2a509776e2ec655a368a5d0e604141986ac62 Author: Arun Mahadevan <arunm@...> Date: 2018-03-12T18:53:50Z STORM-2993: Storm HDFS bolt throws ClosedChannelException when Time rotation policy is used The TimedRotation should synchronize so that the bolt does not attempt to write to a stale writer. ---
[GitHub] storm pull request #2584: BUG-97743: Explicitly add jackson-annotations depe...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2584 BUG-97743: Explicitly add jackson-annotations dependency in pom dependency management This is taken care of in master via jackson-bom dependency. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2985 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2584.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 #2584 commit 3ae4eecb58470f9b2bc3519b187c5fceb8ff14a0 Author: Arun Mahadevan <arunm@...> Date: 2018-03-02T20:01:41Z BUG-97743: Explicitly add jackson-annotations dependency in pom dependency management ---
[GitHub] storm pull request #2570: STORM-2968: Exclude avro and commons-beanutils dep...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2570 STORM-2968: Exclude avro and commons-beanutils dependency from storm-autocreds 1.x version of https://github.com/apache/storm/pull/2569 You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2968-1.x Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2570.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 #2570 commit cdbd35373ccfdec6e174d48368e5a2578555ef34 Author: Arun Mahadevan <arunm@...> Date: 2018-02-20T20:23:20Z STORM-2968: Exclude avro and commons-beanutils dependency from storm-autocreds ---
[GitHub] storm pull request #2569: STORM-2968: Exclude avro and commons-beanutils dep...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2569 STORM-2968: Exclude avro and commons-beanutils dependency from storm-autocreds You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2968 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2569.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 #2569 commit de1103530618eea54240a7c244b5855e1c7025eb Author: Arun Mahadevan <arunm@...> Date: 2018-02-20T20:23:20Z STORM-2968: Exclude avro and commons-beanutils dependency from storm-autocreds ---
[GitHub] storm pull request #2568: STORM-2967: Upgrade jackson to latest version
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2568 STORM-2967: Upgrade jackson to latest version You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2967-1.x Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2568.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 #2568 commit da6a3f9fc159762b61364f1a8101432e25ef7128 Author: Arun Mahadevan <arunm@...> Date: 2018-02-20T20:09:59Z STORM-2967: Upgrade jackson to latest version ---
[GitHub] storm pull request #2567: STORM-2967: Upgrade jackson to latest version
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2567 STORM-2967: Upgrade jackson to latest version You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2967 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2567.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 #2567 commit c917c8a660fd5053c9431ebbc984eb996bf920cf Author: Arun Mahadevan <arunm@...> Date: 2018-02-20T19:36:03Z STORM-2967: Upgrade jackson to latest version ---
[GitHub] storm issue #2556: STORM-2946: Upgrade to HBase 2.0
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2556 @ptgoetz the changes looks good. 1. Can you share the results of testing these changes with HBase 2.0 2. Are these changes compatible with HBase 1.x ? Can we try running the patched bolt with HBase 1.x? 3. Based on the result of 2, we may need to maintain separate storm-hbase modules. Are there any other alternatives ? ---
[GitHub] storm issue #2555: STORM-2841 Use extended class instead of partial mock for...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2555 +1 ---
[GitHub] storm issue #2547: Storm 2913 2914 1.x
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2547 +1 ---
[GitHub] storm issue #2532: STORM-2912 Revert optimization of sharing tick tuple (1.x...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2532 +1 ---
[GitHub] storm issue #2533: STORM-2912 Revert optimization of sharing tick tuple
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2533 +1 Apparently the topology needs to use tick tuple to see the issue. Ran RollingTopWords which uses tick-tuples and can clearly observe the difference. **Before patch** https://user-images.githubusercontent.com/6792890/35416447-f6a09fb4-01dd-11e8-9d73-d4f736865a8b.png;> **After patch** https://user-images.githubusercontent.com/6792890/35416453-feab19fa-01dd-11e8-95c7-901d4b9bc9bb.png;> ---
[GitHub] storm issue #2533: STORM-2912 Revert optimization of sharing tick tuple
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2533 Ran the FastWordCount with and without this patch, the number dont look much different. FastWordcount is expected not to see any difference ? Maybe we can wait for Alexandre's results. Storm 1.2.0 RC1 ``` uptime: 30 acked: 8180 avgLatency: 3218.882640586797 acked/sec: 272.7 failed: 0 uptime: 60 acked: 8240 avgLatency: 3296.849514563107 acked/sec: 137.34 failed: 0 uptime: 90 acked: 2380 avgLatency: 478.61344537815125 acked/sec: 26.443 failed: 0 uptime: 121 acked: 3420 avgLatency: 1493.2397660818713 acked/sec: 28.264462809917354 failed: 0 uptime: 151 acked: 3420 avgLatency: 1493.2397660818713 acked/sec: 22.649006622516556 failed: 0 uptime: 181 acked: 9680 avgLatency: 4966.5268595041325 acked/sec: 53.48066298342541 failed: 0 uptime: 211 acked: 9680 avgLatency: 4966.5268595041325 acked/sec: 45.87677725118483 failed: 0 uptime: 241 acked: 2140 avgLatency: 569.7102803738318 acked/sec: 8.879668049792532 failed: 0 uptime: 271 acked: 5620 avgLatency: 2827.6298932384343 acked/sec: 20.7380073800738 failed: 0 uptime: 301 acked: 5620 avgLatency: 2827.6298932384343 acked/sec: 18.67109634551495 failed: 0 ``` With the patch ``` uptime: 30 acked: 3280 avgLatency: 717.0731707317074 acked/sec: 109.33 failed: 0 uptime: 60 acked: 3280 avgLatency: 717.0731707317074 acked/sec: 54.664 failed: 0 uptime: 90 acked: 3020 avgLatency: 1077.8543046357615 acked/sec: 33.56 failed: 0 uptime: 120 acked: 11380 avgLatency: 5864.984182776801 acked/sec: 94.83 failed: 0 uptime: 150 acked: 11380 avgLatency: 5864.984182776801 acked/sec: 75.86 failed: 0 uptime: 180 acked: 5640 avgLatency: 2464.45390070922 acked/sec: 31.332 failed: 0 uptime: 210 acked: 5640 avgLatency: 2464.45390070922 acked/sec: 26.857142857142858 failed: 0 uptime: 240 acked: 10120 avgLatency: 3743.3379446640315 acked/sec: 42.164 failed: 0 uptime: 270 acked: 11600 avgLatency: 5629.23448275862 acked/sec: 42.96296296296296 failed: 0 uptime: 300 acked: 11600 avgLatency: 5629.23448275862 acked/sec: 38.664 failed: 0 ``` ---
[GitHub] storm issue #2530: STORM-2907: Exclude curator dependencies from storm-core ...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2530 This is so that users could include the "external/storm-autocreds" directory into their class path and have the delegation token mechanism for the storm hdfs, hbase and hive connectors work out of the box. ---
[GitHub] storm pull request #2530: STORM-2907: Exclude curator dependencies from stor...
GitHub user arunmahadevan opened a pull request: https://github.com/apache/storm/pull/2530 STORM-2907: Exclude curator dependencies from storm-core in storm-autocreds pom storm-autocreds brings in the curator 4.0 jars via transitive dependency of storm-core. Even though storm-core is listed as provided scope, the app assembler plugin puts the dependency (curator 4.0) into external/storm-autocreds directory. This conflicts with the storm-druid tranquility library which depends on curator 2.6.0. There could be other conflicts in storm-autocreds is included in class path. Excluding curator dependency from storm-core resolves this. You can merge this pull request into a Git repository by running: $ git pull https://github.com/arunmahadevan/storm STORM-2907 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2530.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 #2530 commit deb960fd43e5d1be26fa68e6c4571d24848c587f Author: Arun Mahadevan <arunm@...> Date: 2018-01-23T22:13:20Z STORM-2907: Exclude curator dependencies from storm-core in storm-autocreds pom ---
[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2203 +1 again. @HeartSaVioR , based on the TVL numbers I interpret that the performance numbers (throughput and latency) are comparable to 1.x branch. In that case can we merge this patch? If we find any other issues during RC testing we can take a call. ---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2519 +1 ---
[GitHub] storm pull request #2519: STORM-2903: Fix possible NullPointerException in A...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2519#discussion_r162705370
--- Diff:
external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java
---
@@ -215,9 +215,17 @@ private void addTokensToUGI(Subject subject) {
if (allTokens != null) {
for (Token token :
allTokens) {
try {
+
+if (token == null) {
+LOG.debug("Ignoring null token");
+continue;
+}
+
LOG.debug("Current user: {}",
UserGroupInformation.getCurrentUser());
-LOG.debug("Token from credential: {} /
{}", token.toString(),
-
token.decodeIdentifier().getUser());
+LOG.debug("Token from Credentials : {}",
token);
+
+if (token.decodeIdentifier() != null)
--- End diff --
@omkreddy , toString already handles it and just printing the token would
take care of decoding and printing the user information.
https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/Token.java#L429
---
[GitHub] storm pull request #2519: MINOR: Fix possible NullPointerException in Abstra...
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2519#discussion_r162679780
--- Diff:
external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java
---
@@ -216,8 +216,7 @@ private void addTokensToUGI(Subject subject) {
for (Token token :
allTokens) {
try {
LOG.debug("Current user: {}",
UserGroupInformation.getCurrentUser());
-LOG.debug("Token from credential: {} /
{}", token.toString(),
-
token.decodeIdentifier().getUser());
+LOG.debug("Extracted Token from
Credentials : {}", token);
--- End diff --
It appears that the token.toString() does some decoding so might print the
user info.
---
[GitHub] storm issue #2519: MINOR: Fix possible NullPointerException in AbstractAutoC...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2519 +1, Thanks for the patch. Can you also associate it with a JIRA and raise a patch for master branch as well? ---
[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2203 @ptgoetz can you squash the commits and also add some user documentation for this? ---
[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2203 @ptgoetz @HeartSaVioR , given that @revans2 was ok with the patch and the performance concerns have been addressed, I suggest we merge this in and start the RC process for Storm 1.2 release going. Meanwhile if @revans2 has additional comments, we can address it before the final RC. ---
[GitHub] storm issue #2515: STORM-2900 Always return non null collection for config k...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2515 +1 ---
[GitHub] storm issue #2516: STORM-2900 Always return non null collection for config k...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2516 +1 ---
[GitHub] storm issue #2499: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2499 @HeartSaVioR , are we ok to merge this in 1.x while we wait for the right fix for #2498 ? ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 I think "option 2" is good for Storm 2.0 and it may be ok have the users rebuild their topologies that relies on storm-druid and use the relocated package names. With "option 4", users have to ensure that the relocated storm-client jar is present in any storm class paths instead of the regular storm-client jar, so may be tricky. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 @revans2 , the latest version of tranquility-core available is 0.8.2 which has this curator 2.6.0 transitive dependency. I think the storm-druid unit tests would use the 2.6.0 version of curator since the version is explicitly mentioned in the storm-druid pom. If we decide to not relocate the storm's curator dependencies, storm-druid with this patch will have a curator version conflict at runtime and may fail. The other option could be to shade storm-druid and relocate its curator dependencies, but it may break the existing topologies using storm-druid and they would need to use the relocated package names. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 @HeartSaVioR , any reason why we dont relocate in 2.0.0 vs 1.x ? If not users would hit issues with conflicting versions (like curator). ---
