You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by ptgoetz <gi...@git.apache.org> on 2015/11/17 15:55:30 UTC
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
GitHub user ptgoetz opened a pull request:
https://github.com/apache/storm/pull/887
STORM-1213: Remove sigar binaries from source tree
See https://issues.apache.org/jira/browse/STORM-1213 for description.
Also removes the LICENSE entry for sigar, since I believe that is unnecessary since it is Apache licensed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ptgoetz/storm storm-metrics-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/887.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 #887
----
commit 83eced3329dc62c685aa6784792506766343fd52
Author: P. Taylor Goetz <pt...@gmail.com>
Date: 2015-11-16T22:17:19Z
remove sigar binaries from source tree and delete unnecessary LICENSE entry
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157862181
Even with changing the directory I am getting
```
java.lang.UnsatisfiedLinkError: org.hyperic.sigar.Sigar.getPid()J
at org.hyperic.sigar.Sigar.getPid(Native Method) ~[stormjar.jar:0.11.0-SNAPSHOT]
at org.apache.storm.metrics.sigar.CPUMetric.<init>(CPUMetric.java:38) ~[stormjar.jar:0.11.0-SNAPSHOT]
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:1.8.0]
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:1.8.0]
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:1.8.0]
at java.lang.reflect.Constructor.newInstance(Constructor.java:408) ~[?:1.8.0]
at java.lang.Class.newInstance(Class.java:433) ~[?:1.8.0]
at backtype.storm.utils.Utils.newInstance(Utils.java:91) ~[storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.metric.SystemBolt.registerMetrics(SystemBolt.java:150) ~[storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.metric.SystemBolt.prepare(SystemBolt.java:143) ~[storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.daemon.executor$fn__5157$fn__5170.invoke(executor.clj:777) ~[storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.util$async_loop$fn__556.invoke(util.clj:480) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at clojure.lang.AFn.run(AFn.java:22) [clojure-1.7.0.jar:?]
at java.lang.Thread.run(Thread.java:744) [?:1.8.0]
```
We need to do some more work to understand the difference between this patch and what we were doing before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-158184985
@revans2 I couldn't get the maven dependency plugin to do what I wanted, so I changed tactics. I switched to using the maven antrun plugin to download and unpack the sigar native binaries. It downloads the archive to the local maven repository alongside the other sigar dependencies so it only needs to download it once.
The good news is that allowed me to use the full binary archive, so it includes support for windows and other OSes/architectures. On the down side it's somewhat brittle compared to using "pure" maven, and IMO is kind of ugly. It's also hard-coded to use google code, which is supposedly going away at some point, but that's the only place I could find the archive with a published checksum.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/887
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157869384
@revans2 That directory structure is what's in master right now:
https://github.com/apache/storm/tree/master/external/storm-metrics/src/main/resources/resources
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157873921
In master https://github.com/apache/storm/tree/master/external/storm-metrics/src/main/resources gets mapped into the base directory of the jar. So https://github.com/apache/storm/tree/master/external/storm-metrics/src/main/resources/resources goes to jar://storm-merics.jar/resources/
For my tests I launch a single node cluster
```
$ mvn clean install -DskipTests -Drat.skip
$ cd storm-dist/binary
$ mvn clean package
$ tar -xzvf ./target/*.tar.gz
$ cd apache-strom*
$ ./bin/storm dev-zookeeper & #nimbus, ui, logviewer, supervisor,
$ ./bin/storm jar ./examples/storm-starter/storm-starter-topologies-0.11.0-SNAPSHOT.jar storm.starter.ThroughputVsLatency 2000 1 5
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157876251
yes that is it. When I moved `resources/libsigar-universal64-macosx-1.6.4.dylib` to `resources/libsigar-universal64-macosx.dylib` and reran it all worked.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157880047
@revans2 Ah... okay. So we need to change `resources/resources` to just `resources`, and remove the `-1.6.4` from the libraries, correct?
That should be doable with the dependency plugin.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157874959
I just did a diff of the contents of the two jars, old and new(this patch plus me moving the native libraries) and I came up with the following. Around the max native pieces
```
Only in ./new/resources: libsigar-universal64-macosx-1.6.4.dylib
Only in ./old/resources: libsigar-universal-macosx.dylib
Only in ./old/resources: libsigar-universal64-macosx.dylib
```
It looks like the version number in the dylib might be messing up the linking somehow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157858618
Sorry about the comments after giving it a +1. I manually ran LatencyVsThroughput and saw that it was not working.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157705671
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-158094151
@ptgoetz Yes you are correct about the changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157868784
@revans2 How are you running it (i.e. local/remote/unit test)? My first thought is that we may just need to bind the dependency plugin to a different maven lifecycle.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-158199509
@ptgoetz
+1 the changes look good and my manual tests all pass.
I Agree the it kind of ugly. Ideally what we need to do is to talk to sigar and see if they can update their native jar to do what we want. It should be fairly simple to have them remove the version number from the jar.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/887#discussion_r45257318
--- Diff: external/storm-metrics/pom.xml ---
@@ -55,4 +62,28 @@
<scope>provided</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <version>2.10</version>
+ <executions>
+ <execution>
+ <id>unpack-dependencies</id>
+ <phase>generate-resources</phase>
+ <goals>
+ <goal>unpack-dependencies</goal>
+ </goals>
+ <configuration>
+ <outputDirectory>${project.build.directory}/classes/resources/resources</outputDirectory>
--- End diff --
This is the line causing the issue. It should just be `<outputDirectory>${project.build.directory}/classes/resources</outputDirectory>`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/887#discussion_r45257261
--- Diff: external/storm-metrics/pom.xml ---
@@ -48,6 +48,13 @@
</exclusions>
</dependency>
<dependency>
+ <groupId>org.fusesource</groupId>
+ <artifactId>sigar</artifactId>
+ <version>1.6.4</version>
--- End diff --
We should also make this version a property or set in the parent pom, because there are multiple packages tied to this same version. And we should remove the comment on line 39, as it is not needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157843581
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157827199
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1213: Remove sigar binaries from source ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/887#issuecomment-157857431
Actually I just tried to run this and it looks like the libraries are under `resources/resources/` instead of just `resources/` because of this I am getting linking errors when trying to use the metrics.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---