Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193077896
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+
+
+
+http://maven.apache.org/POM/4.0.0;
+ xmlns:xsi="http://www.w3.org
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193077799
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+
+
+
+http://maven.apache.org/POM/4.0.0;
+ xmlns:xsi="http://www.w3.org
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2700
The travis failure looks unrelated.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2691
I am planning right now to get STORM-2882 in first, and then I will come
back and do as much manual testing as possible for the different components,
and update thing accordingly.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
@HeartSaVioR I have addressed the other review comments, but I could not
find a place in the documentation for instructions on how to do a release. I
agree that we want to publish the shaded-deps
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2701#discussion_r192757264
--- Diff: storm-client/src/jvm/org/apache/storm/utils/VersionedStore.java
---
@@ -25,9 +25,17 @@
private String _root;
-public
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2669
Actually we just hit this in production so thanks again @pczb for finding
and fixing this.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
Shading here is different from how it is in 1.x. For this one there is a
separate package that creates a shaded uber jar. The code inside storm-client
and storm-server that need to use
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
Dependencies for the client in 1.2.1
asm
clojure - removed in 2.x
disruptor - removed by this patch
gmetric4j
kryo
log4j2
log4j1.2-api
metrics-core
metrics
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2698
STORM-2882: shade storm-client dependencies
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-2882
Alternatively
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2694
STORM-3061: Upgrade version of hdrhistogram
not too much is using hdrhistogram.
storm-metrics,
storm-loadgen,
storm-starter,
storm-elasticsearch,
storm-elasticsearch
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2691
Yes that was the plan. there is a lot that depends on storm-autocreds and
I would like to understand it all better before I try to clean it up.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2691
@arunmahadevan I am happy to try and split up autocreds to make that
happen, but it is a much larger job than what is currently for this. If you
are fine with waiting I would rather file a follow
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2691#discussion_r190432849
--- Diff: pom.xml ---
@@ -294,7 +294,7 @@
0.14.0
2.6.1
${hadoop.version}
-1.1.12
+1.4.4
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2691#discussion_r190428653
--- Diff: pom.xml ---
@@ -294,7 +294,7 @@
0.14.0
2.6.1
${hadoop.version}
-1.1.12
+1.4.4
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2691
STORM-3061: Update version of hbase
This updates the version of hbase used and cleans up some of the
dependencies.
The biggest change besides updating the version is that we remove
storm
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2688#discussion_r190401760
--- Diff: bin/storm.py ---
@@ -705,7 +705,6 @@ def
nimbus(klass="org.apache.storm.daemon.nimbus.Nimbus"):
cppaths = [CLUSTE
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2690
STORM-3061: Clean up some storm-druid dependencies
`mvn dependency:tree` showed that the only real dependency changes were for
scala and jackson that went from 2.4.6 to 2.9.4.
I am
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2687
Yes I reran the integration tests manually and they all passed.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2687
The integration test failures look unrelated to this change.
---
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2689
STORM-3061: rocket, jms, and mqtt updates
This updates some dependencies for the jms examples, mqtt examples,
rocketmq examples, and updates activemq implementation used for testing jms,
rocketmq
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2687#discussion_r190287991
--- Diff: storm-client/src/genthrift.sh ---
@@ -17,7 +17,7 @@
rm -rf gen-javabean gen-py py
rm -rf jvm/org/apache/storm/generated
-thrift
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2688
STORM-3061: Remove unused core dependencies
This is for disruptor and java.jmx
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2687#discussion_r190067973
--- Diff: storm-client/src/genthrift.sh ---
@@ -17,7 +17,7 @@
rm -rf gen-javabean gen-py py
rm -rf jvm/org/apache/storm/generated
-thrift
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2687
STORM-3061: thrift 0.11
This moves to thrift 0.11
The only files changed are pom.xml and genthrift.sh. The other files are
all generated.
You can merge this pull request into a Git
Github user revans2 closed the pull request at:
https://github.com/apache/storm/pull/2675
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2675
I'll try to break it down to more manageable chunks that are split up by
component as much as possible. There are a lot of the dependencies that cross
boundaries though I will call those out
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2684#discussion_r189076711
--- Diff:
storm-client/src/jvm/org/apache/storm/utils/WrappedAuthorizationException.java
---
@@ -0,0 +1,17 @@
+package org.apache.storm.utils
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2669
Once you make the corresponding changes on #2661 I would be happy to merge
them both in.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2675
The failure looks like it might be related to #2674
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2618
@danny0405 Sorry about the long delay. I also got rather busy with other
things.
My personal choice would be a combination of 1 and 2. We have run into an
issue internally where very
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2633
@ghajos any plans to address the question from @srdo
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2651
@kishorvpatil any update on the comments from @HeartSaVioR ?
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2661
@pczb I have the same comments that I put on the master branch version of
the pull request.
---
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2669#discussion_r188055495
--- Diff: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java
---
@@ -451,7 +451,6 @@ public int getPort() {
public void close
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2669#discussion_r188056330
--- Diff:
storm-client/src/jvm/org/apache/storm/messaging/netty/Context.java ---
@@ -65,20 +65,8 @@ public synchronized IConnection bind(String storm_id
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2669#discussion_r188055726
--- Diff:
storm-client/src/jvm/org/apache/storm/messaging/netty/Context.java ---
@@ -65,20 +65,8 @@ public synchronized IConnection bind(String storm_id
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2669
The original code added this so when shutting down the context we could be
sure that all ongoing connections were also terminated as cleanly as possible.
After this change that is only true
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2675
The travis failure is an interesting one, sql-core is failing, but none of
the tests are failing, the junit JVM exist badly as a part of shutdown. I have
not been able to reproduce it locally
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2675
STORM-3061: Upgrade lots of dependencies
This upgrades lots of dependencies, including thrift. The thrift changes
are based off of an earlier patch done by @arunmahadevan
Please skip
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2647
@danny0405 the failure is a known race condition around netty and is not
related to this change.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2664
@arunmahadevan and @srdo
https://issues.apache.org/jira/browse/STORM-2882 is for adding back in
shading to the storm client. I am happy to take that up next. My real concern
is the long
Github user revans2 closed the pull request at:
https://github.com/apache/storm/pull/2664
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2647
@danny0405 I added in the comments about thread safety like you suggested.
---
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2647#discussion_r186756808
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/Cluster.java ---
@@ -763,6 +773,7 @@ public void setAssignments
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2647#discussion_r186754134
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/Cluster.java ---
@@ -48,6 +49,9 @@
public class Cluster implements
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2664
STORM-2884: Remove storm-druid
STORM-2884 is an issue with differences in the version of dependencies in
druid tranquility conflicting with versions in storm. I personally think the
right way
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2647
Oh I forgot I also added back in something I messed up before and added
back in anti-affinity to GRAS.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2647
@danny0405 @kishorvpatil With some recent changes to master my patch
started to fail with some checkstyle issues. I have rebased and fixed all of
the issues. Please take a look again, specifically
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2657
I am getting a failure now of
```
[ERROR] Failed to execute goal
org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (validate) on
project storm-server: You have 792 Checkstyle
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2647
@danny0405 In my tests
`TestResourceAwareScheduler.testLargeTopologiesCommon` went from about 7 mins
to about 7 seconds. For
`TestResourceAwareScheduler.testLargeTopologiesOnLargeClustersGras` I
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2641
Still +1
---
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2647
STORM-3040: Improve scheduler performance
There are a lot of different scheduler improvements. Mostly these are
either caching, storing data in multiple ways so we can look it up quickly
Github user revans2 closed the pull request at:
https://github.com/apache/storm/pull/2630
---
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2634#discussion_r182750384
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -3032,9 +3019,19 @@ public void submitTopologyWithOpts(String
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2634#discussion_r182747281
--- Diff: docs/Resource_Aware_Scheduler_overview.md ---
@@ -184,6 +184,10 @@ The user can set some default configurations for the
Resource Aware Scheduler
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2635#discussion_r182745958
--- Diff:
external/storm-autocreds/src/main/java/org/apache/storm/hbase/security/HBaseSecurityUtil.java
---
@@ -52,24 +54,27 @@ private HBaseSecurityUtil
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2630
STORM-3024: Allow for scheduling to happen in the background.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2622
@HeartSaVioR thanks for the review. I fixed your concerns by making them
all ConcurrentHashMaps and adding a note about why they need to be that. I
could not find a good way to remove
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2622
@HeartSaVioR
Great catch I forgot to update the normal has Map/HashMap to a
ConcurrentHashMap. Yes the guarantees of ConcurrentMap allow for retry and we
do have side effects in some
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179570567
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179568920
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179569824
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179570190
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179569508
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179571966
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2623#discussion_r179569132
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -477,45 +414,136
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2618
OK good I do understand the problem.
There really are a few ways that I see we can make the stack trace much
less likely to come out in the common case. The following are in my preferred
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2622
Thanks for the review @danny0405
This was not trying to fix STORM-2905. This was a separate race condition
I found when reviewing your pull request for STORM-2905.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2618
@danny0405
I just created #2622 to fix the race condition in AsyncLocalizer. It does
conflict a lot with this patch, so I wanted to make sure you saw it and had a
chance to give feedback
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2622
STORM-3020: fix possible race condition in AsyncLocalizer
There were a number of places in AsyncLocalizer that were using
synchronized to try and protect some maps. When we added in support
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2618
Just FYI I files STORM-3020 to address the race that I just found.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2618
@danny0405
{{updateBlobs}} does not need to be guarded by a lock. This is what I was
talking about with the code being complex.
{{requestDownloadBaseTopologyBlobs}} is protected
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2618#discussion_r178852812
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Slot.java ---
@@ -497,7 +497,6 @@ static DynamicState
cleanupCurrentContainer
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2618
I am trying to understand the reasons behind this change. Is this jira
just to remove an exception that shows up in the logs? Or is that exception
actually causing a problem?
The reason I
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2618#discussion_r178568732
--- Diff:
storm-server/src/main/java/org/apache/storm/localizer/LocalizedResourceRetentionSet.java
---
@@ -94,17 +93,17 @@ public void cleanup
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2618#discussion_r178567096
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Slot.java ---
@@ -497,7 +497,6 @@ static DynamicState
cleanupCurrentContainer
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2591
This works and I am OK with merging it in +1. But the implementation of
deserializing the hooks twice feels really odd and non-intuitive to me. But
this is not your problem so it should be fine
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2604
The docs are not identical for master, so I will manually make some updates
to master, which is allowed without code review.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2607
@jnioche I was able to cherry-pick #2609 to all of the branches, but I
missed this one. Please close this pull request, and sorry for the
inconvenience.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2433
I am +1, but a little nervous that the tests are failing consistently on
travis in exactly the same way, but never on my laptop, but I think we can work
that out later if it is a real issue.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2433
@danny0405 the changes look good, the conflicts are minimal and I think the
test failures are spurious. I am +1 for merging this in. Please rebase/squash
the commits, resolve the minor conflicts
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2433
@danny0405 Sorry about how long this has taken. I am back from vacation
now. I will take a look at the patch again, and if the conflicts are small
hopefully we can merge it in today or tomorrow.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2433
@danny0405 I created https://github.com/danny0405/storm/pull/2 to add in
authorization support for the supervisor and the new nimbus APIs.
---
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2433
OK I'll try to put up another pull request to cover basic auth for the
supervisor.
---
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173232562
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -4215,7 +4474,48 @@ public boolean isTopologyNameAllowed(String name
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173232056
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java
---
@@ -234,6 +295,60 @@ public void launchDaemon
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173219240
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -319,14 +344,17 @@ private static StormBase make(TopologyStatus
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173220578
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/ReadClusterState.java
---
@@ -293,14 +268,15 @@ public synchronized void run
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173216785
--- Diff:
storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java ---
@@ -295,7 +295,11 @@ private Scope calculateScope(Map<Inte
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173218011
--- Diff: storm-client/src/jvm/org/apache/storm/utils/SupervisorClient.java
---
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173213537
--- Diff:
storm-client/src/jvm/org/apache/storm/cluster/IStormClusterState.java ---
@@ -233,23 +249,11 @@
* @return the id of the topology or null
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173217779
--- Diff: storm-client/src/jvm/org/apache/storm/utils/SupervisorClient.java
---
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173213078
--- Diff:
storm-client/src/jvm/org/apache/storm/cluster/IStormClusterState.java ---
@@ -94,6 +108,8 @@
@Deprecated
List
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173207181
--- Diff:
storm-client/src/jvm/org/apache/storm/assignments/InMemoryAssignmentBackend.java
---
@@ -0,0 +1,152 @@
+/**
+ * Licensed to the Apache
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173214066
--- Diff:
storm-client/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java ---
@@ -745,6 +828,7 @@ public void disconnect
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173210647
--- Diff:
storm-client/src/jvm/org/apache/storm/assignments/InMemoryAssignmentBackend.java
---
@@ -0,0 +1,152 @@
+/**
+ * Licensed to the Apache
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173213324
--- Diff:
storm-client/src/jvm/org/apache/storm/cluster/IStormClusterState.java ---
@@ -233,23 +249,11 @@
* @return the id of the topology or null
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173215759
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/Worker.java
---
@@ -392,6 +401,30 @@ public void establishLogSettingCallback
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2433#discussion_r173215187
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/Worker.java
---
@@ -392,6 +401,30 @@ public void establishLogSettingCallback
301 - 400 of 3470 matches
Mail list logo
