[GitHub] storm issue #2913: STORM-3290: Split configuration for storm-kafka-client Tr...

2018-11-29 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2913 +1 ---

[GitHub] storm issue #2706: STORM-3097: deprecate storm-druid (1.x)

2018-06-07 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2706 +1 ---

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2698 +1 @srdo You asked what the maven release commands were. They are: ``` mvn release:prepare -P dist mvn release:perform -P dist ``` Note that the process modifies

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

2018-05-11 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2639 Still +1. Thanks @arunmahadevan ---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

2018-05-11 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2639 Looks good to me. +1 I was wondering why you removed the `distributed` flag, and realized it was never properly implemented! For it to work, we would need the following method override

[GitHub] storm issue #2666: STORM-2988 Error on initialization of server mk-worker

2018-05-09 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2666 +1 ---

[GitHub] storm issue #2665: STORM-2988 Error on initialization of server mk-worker

2018-05-09 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2665 +1 ---

[GitHub] storm issue #2665: STORM-2988 Error on initialization of server mk-worker

2018-05-08 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2665 @HeartSaVioR Agreed, but those issues can be addressed outside the scope of this bug (i.e. separate JIRA). For now we should focus on getting this fix in since it is currently holding up the release. ---

[GitHub] storm issue #2665: STORM-2988 Error on initialization of server mk-worker

2018-05-07 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2665 > @ptgoetz , any reason to use a different constant while this config exists? And JmxStormReporter seem to use the constants from Config here and here ? @arunmahadevan, to keep t

[GitHub] storm pull request #2665: STORM-2988 Error on initialization of server mk-wo...

2018-05-07 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2665#discussion_r186580893 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/JmxStormReporter.java --- @@ -67,7 +68,7 @@ public void prepare(MetricRegistry

[GitHub] storm pull request #2665: STORM-2988 Error on initialization of server mk-wo...

2018-05-07 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2665#discussion_r186578267 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/JmxStormReporter.java --- @@ -67,7 +68,7 @@ public void prepare(MetricRegistry

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

2018-04-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2518 +1 ---

[GitHub] storm issue #2300: STORM-2691: Make storm-kafka-client implement the Trident...

2018-02-27 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2300 +1 ---

[GitHub] storm issue #2556: STORM-2946: Upgrade to HBase 2.0

2018-02-21 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2556 @HeartSaVioR @arunmahadevan I tested this manually against HBase 2.0.0 beta-1 and HBase 1.1.2 using the examples as well as some custom code covering trident and core storm read/write. All

[GitHub] storm issue #2556: STORM-2946: Upgrade to HBase 2.0

2018-02-16 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2556 @arunmahadevan Yes, I will share the results of my testing and will test for backward compatibility. I don't have high hopes for backward compatibility based on https://hbase.apach

[GitHub] storm issue #2556: STORM-2946: Upgrade to HBase 2.0

2018-02-13 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2556 > Did you test the patch manually with HBase 2.0 beta 1? Not extensively. The point on this JIRA/PR is to raise awareness that we should look at targeting new ecosystem versions. This ho

[GitHub] storm pull request #2556: STORM-2946: Upgrade to HBase 2.0

2018-02-13 Thread ptgoetz
GitHub user ptgoetz opened a pull request: https://github.com/apache/storm/pull/2556 STORM-2946: Upgrade to HBase 2.0 https://issues.apache.org/jira/browse/STORM-2946 We may want to discuss changes like this in the email thread about independently versioned "ext

[GitHub] storm-site issue #2: Update dependencies

2018-02-12 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm-site/pull/2 +1 ---

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

2018-02-07 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2550 +1 successfully built for me and all tests passed. As far as squashing commits my opinion has always been if we do squash, it should be done when merging. There are a lot of cases where

[GitHub] storm issue #2549: STORM-2936 Overwrite latest storm-kafka-client 1.x-branch...

2018-02-06 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2549 +1 ---

[GitHub] storm issue #2547: Storm 2913 2914 1.x

2018-02-05 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2547 +1 ---

[GitHub] storm issue #2541: STORM-2918 Update Netty version

2018-02-01 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2541 +1 ---

[GitHub] storm issue #2532: STORM-2912 Revert optimization of sharing tick tuple (1.x...

2018-01-25 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2532 +1 Nice catch. Agree on performance -- tick tuples are sent infrequently enough that the optimization isn't necessary. ---

[GitHub] storm issue #2526: STORM-2904: Document Metrics V2

2018-01-22 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2526 @HeartSaVioR Fixed. ---

[GitHub] storm pull request #2526: STORM-2904: Document Metrics V2

2018-01-22 Thread ptgoetz
GitHub user ptgoetz opened a pull request: https://github.com/apache/storm/pull/2526 STORM-2904: Document Metrics V2 Documentation only. No code changes. You can merge this pull request into a Git repository by running: $ git pull https://github.com/ptgoetz/storm

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-21 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @HeartSaVioR Absolutlely! I just want to make sure we are all on board with the changes. ---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-21 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 It seems like there is growing consensus that performance is good. Are there any objections to merging this? ---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-17 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 Are there any additional changes you'd like to see for this? I'd like to move forward with a 1.2 release as well as start porting this to the master branch. ---

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-12 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161356664 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,181 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-12 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161324136 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,181 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-12 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161301526 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,181 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-12 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161292783 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,181 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-12 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161291952 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,181 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-11 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161139613 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/TaskMetrics.java --- @@ -0,0 +1,81 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-11 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 When reporting benchmark results, we should include OS patch level. The recent wave of patches will likely mess with benchmarks. ---

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-11 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r161087710 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -265,6 +265,7 @@ :stats (mk-executor-stats <> (sampling-rate stor

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-11 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 Could you take another look when you have a chance? ---

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-10 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160805811 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -446,7 +451,7 @@ (.ack spout msg-id) (task/apply-hooks (:user

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-10 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160777488 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -446,7 +451,7 @@ (.ack spout msg-id) (task/apply-hooks (:user

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-10 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 @HeartSaVioR Moved to `StringBuilder` and replaced executorId with taskId. ---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2018-01-10 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @HeartSaVioR @revans2 One obvious optimization worth trying is replacing `String.format()` with a `StringBuilder`. `String.format()` is cleaner visually, but much slower. I'll

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-09 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160513274 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -446,7 +451,7 @@ (.ack spout msg-id) (task/apply-hooks (:user

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-09 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160508705 --- Diff: storm-core/src/jvm/org/apache/storm/nimbus/DefaultTopologyValidator.java --- @@ -17,15 +17,49 @@ */ package org.apache.storm.nimbus

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-09 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160506051 --- Diff: storm-core/src/jvm/org/apache/storm/task/TopologyContext.java --- @@ -386,4 +388,28 @@ public ReducedMetric registerMetric(String name, IReducer

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-09 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160505473 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,156 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-09 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r160499395 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-03 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r159519245 --- Diff: storm-core/src/jvm/org/apache/storm/utils/DisruptorQueue.java --- @@ -65,7 +66,7 @@ private static final Object INTERRUPT = new Object

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2018-01-03 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r159515622 --- Diff: storm-core/src/jvm/org/apache/storm/utils/DisruptorQueue.java --- @@ -65,7 +66,7 @@ private static final Object INTERRUPT = new Object

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-12-22 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @srdo Thanks for the review. I think I've addressed your comments in the latest commit. ---

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-22 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r158549262 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/ScheduledStormReporter.java --- @@ -0,0 +1,90 @@ +/** + * Licensed to the Apache

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-22 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r158549210 --- Diff: storm-core/src/jvm/org/apache/storm/validation/ConfigValidation.java --- @@ -493,6 +493,46 @@ public void validateField(String name, Object o

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-22 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r158549192 --- Diff: storm-core/src/jvm/org/apache/storm/utils/DisruptorQueue.java --- @@ -418,12 +430,19 @@ public DisruptorQueue(String queueName, ProducerType type

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-22 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r158545079 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/StormReporter.java --- @@ -0,0 +1,32 @@ +/** + * Licensed to the Apache Software

[GitHub] storm issue #2462: STORM-2858: Fix worker-launcher build by erroring out if ...

2017-12-21 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2462 +1 ---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-12-20 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @arunmahadevan Rebased. ---

[GitHub] storm issue #2469: STORM-2861: Explicit reference kafka-schema-registry-clie...

2017-12-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2469 @vesense I tried to reproduce this by deleting Kafka-avro-serializer from my local maven repository, but the build succeeded. Can you elaborate a bit? ---

[GitHub] storm issue #2470: [STORM-2690] resurrect invocation of ISupervisor.assigned...

2017-12-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2470 +1 ---

[GitHub] storm issue #2472: [STORM-2690] resurrect invocation of ISupervisor.assigned...

2017-12-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2472 +1 ---

[GitHub] storm issue #2471: [STORM-2690] resurrect invocation of ISupervisor.assigned...

2017-12-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2471 +1 ---

[GitHub] storm issue #2468: [STORM-2690] resurrect invocation of ISupervisor.assigned...

2017-12-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2468 @erikdw Excellent! Thanks bringing this up and helping solve the issue. ---

[GitHub] storm issue #2468: [STORM-2690] resurrect invocation of ISupervisor.assigned...

2017-12-19 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2468 +1 @erikdw Are you planning to port to earlier branches? ---

[GitHub] storm pull request #2458: (1.x) STORM-2854 Expose IEventLogger to make event...

2017-12-14 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2458#discussion_r157091241 --- Diff: storm-core/src/jvm/org/apache/storm/metric/IEventLogger.java --- @@ -31,32 +32,54 @@ /** * A wrapper for the fields that we

[GitHub] storm issue #2459: STORM-2855: Revert to 2017Q4 Ubuntu image in Travis to fi...

2017-12-14 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2459 +1 ---

[GitHub] storm issue #2447: STORM-2845 Drop standalone mode of Storm SQL

2017-12-14 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2447 +1 ---

[GitHub] storm pull request #2458: (1.x) STORM-2854 Expose IEventLogger to make event...

2017-12-14 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2458#discussion_r157056998 --- Diff: storm-core/src/jvm/org/apache/storm/metric/IEventLogger.java --- @@ -31,32 +32,54 @@ /** * A wrapper for the fields that we

[GitHub] storm pull request #2458: (1.x) STORM-2854 Expose IEventLogger to make event...

2017-12-14 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2458#discussion_r157056272 --- Diff: conf/storm.yaml.example --- @@ -72,4 +72,11 @@ # argument: # - endpoint: "metrics-collector.mycompan

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-12-14 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 > Looks like we compose metric name and lookup from REGISTRY every time, even without executor ID and stream ID. I can see more calculation should be done after addressing, but not sure how much

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-12-13 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 @HeartSaVioR I added stream id and executor id to the metrics names, and implemented replacing "." with "_". One consequence of adding stream id

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 Thanks for clarifying. I have a better understanding of what you're saying now. How do you feel about just replacing "." with "_" on all metrics pa

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155864521 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/ScheduledStormReporter.java --- @@ -0,0 +1,90 @@ +/** + * Licensed to the Apache

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155863202 --- Diff: storm-core/src/jvm/org/apache/storm/validation/ConfigValidation.java --- @@ -493,6 +493,46 @@ public void validateField(String name, Object o

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155863192 --- Diff: storm-core/src/jvm/org/apache/storm/validation/ConfigValidation.java --- @@ -493,6 +493,46 @@ public void validateField(String name, Object o

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155856433 --- Diff: storm-core/src/jvm/org/apache/storm/validation/ConfigValidation.java --- @@ -493,6 +493,46 @@ public void validateField(String name, Object o

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155855673 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/ScheduledStormReporter.java --- @@ -0,0 +1,90 @@ +/** + * Licensed to the Apache

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155855300 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/ScheduledStormReporter.java --- @@ -0,0 +1,90 @@ +/** + * Licensed to the Apache

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r155854567 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/reporters/GangliaStormReporter.java --- @@ -0,0 +1,136 @@ +/** + * Licensed to the Apache

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-12-08 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 (cc @HeartSaVioR ) Regarding metadata, parseable metrics names/paths, etc. what do you think of the following approach? In a nutshell, make everything configurable (with sane

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-29 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @HeartSaVioR Agreed/understood. We can squash on merge and cleanup commit messages. This is a feature branch. You can commit directly if you want. ---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-29 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @HeartSaVioR Thanks for the update. I pulled your changes into the metrics_v2 branch. @revans2 I'll start working on naming conventions and disallowing certain delimiter characters. I

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-21 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 Crude test, but illustrates the cost of meters: (code marks a meter | increments a counter from 0 to `Integer.MAX`) ``` *** METER *** Time: 126.39 ops/sec: 16,990,930

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-21 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 Thanks for the update. My first instinct is that it’s usage of Meter on the critical path. Adding anything that “does something” there is going to add some level of overhead

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-20 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @HeartSaVioR I know. ;) I'm thinking more in terms of hardware profile and what other processes are running. ---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-20 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 We really need hardware and environment information. I'd also argue that tests should be run headless. I've seen some benchmarks vary greatly on a MBP depending on what you're do

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-11-20 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 You may have to jog your memory ;), but do you recall the parameters you used for ThroughputVsLatency? I just ran ThroughputVsLatency (with defaults) on an isolated machine (not a

[GitHub] storm issue #2409: STORM-2796: Flux: Provide means for invoking static facto...

2017-11-16 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2409 @roshannaik See latest commit. I merged/fixed your patch. Most of the issues were typos (e.g. "refList" != "reflist"). The other issue was that properties in flux do not

[GitHub] storm issue #2409: STORM-2796: Flux: Provide means for invoking static facto...

2017-11-16 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2409 @HeartSaVioR Yes, I will add assertions, although the tests would still fail without them (the constructor/method won't be found and a IllegalArgumentException will be thrown). @rosha

[GitHub] storm issue #2409: STORM-2796: Flux: Provide means for invoking static facto...

2017-11-15 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2409 @HeartSaVioR Thanks for the review. Addressed your comment and fixed another issue discovered in testing. ---

[GitHub] storm issue #2409: STORM-2796: Flux: Provide means for invoking static facto...

2017-11-10 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2409 I can squash the commits later, but for now keeping the check style commit separate will make review a lot easier. ---

[GitHub] storm pull request #2409: STORM-2796: Flux: Provide means for invoking stati...

2017-11-10 Thread ptgoetz
GitHub user ptgoetz opened a pull request: https://github.com/apache/storm/pull/2409 STORM-2796: Flux: Provide means for invoking static factory methods Note: The guts of this change are in the first commit. The last commit simply addresses check style violations. You can merge

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-09-29 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 Added rudimentary sanity check validation for metrics reporters configs. Because reporter implementations may want to have their own custom config keys, we can't really cover everythi

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-09-29 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2 I agree with you regarding getting the metrics naming correct. In fact I think it's one of the most important aspects. I'd like to get as much input/collaboration from others as p

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-09-29 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @revans2, @HeartSaVioR, @arunmahadevan Addressed review comments. Also note that this is a feature branch, so it's open to pull requests from anyone. If there's anything you

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-09-29 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r141924200 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/Metrics2Utils.java --- @@ -0,0 +1,28 @@ +/** + * Licensed to the Apache Software Foundation

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-09-29 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r141922507 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-09-26 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r141137956 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-09-26 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r141137280 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-09-26 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @HeartSaVioR > We need to document how to use new metrics and its reporter, and also need to have patch for master branch (maybe with removal of metrics V1 public API). A

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-09-26 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r141137596 --- Diff: storm-core/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java --- @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

2017-09-26 Thread ptgoetz
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2203 @arunmahadevan > Currently this patch addresses registering custom metrics and sending the metrics out via reporters and does not send any metrics to nimbus right? Why is disruptor metr

[GitHub] storm pull request #2203: STORM-2153: New Metrics Reporting API

2017-09-26 Thread ptgoetz
Github user ptgoetz commented on a diff in the pull request: https://github.com/apache/storm/pull/2203#discussion_r141131796 --- Diff: examples/storm-starter/src/jvm/org/apache/storm/starter/AnchoredWordCount.java --- @@ -0,0 +1,138 @@ +/** + * Licensed to the Apache

  1   2   3   4   5   6   7   8   >