Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2913
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2706
+1
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2639
Still +1. Thanks @arunmahadevan
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2666
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2665
+1
---
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 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 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 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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2518
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2300
+1
---
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 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 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 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 user ptgoetz commented on the issue:
https://github.com/apache/storm-site/pull/2
+1
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2549
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2547
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2541
+1
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2526
@HeartSaVioR Fixed.
---
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2203
@revans2 @HeartSaVioR Moved to `StringBuilder` and replaced executorId with
taskId.
---
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 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 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 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 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 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 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 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 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 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 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 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 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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2462
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2203
@arunmahadevan Rebased.
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2470
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2472
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2471
+1
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2468
+1
@erikdw Are you planning to port to earlier branches?
---
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 user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2459
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2447
+1
---
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 - 100 of 774 matches
Mail list logo