You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by revans2 <gi...@git.apache.org> on 2018/05/30 20:06:14 UTC
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
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 you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2698.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 #2698
----
commit 95082cec14cd03bc566db2b0e90555446b519082
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-15T15:05:23Z
STORM-2882: Shade Guava and Curator
commit 3f6a43f04e02bb238312fff242eb7c4e499d3fdc
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-15T23:28:20Z
STORM-2882: Moved Servlet API to Server
commit d20757b08b21b2b714e02a7f17a793b11052f2cd
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-16T00:19:02Z
STORM-2882: Change Auth Methods to match standards
commit df0fd118dd0e953df25bb7ad3d656cbcccd84626
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-16T12:27:25Z
Missed 2 files
commit 11d5f50f9737f173d764b1cdfd7010274026b6b0
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-16T12:32:37Z
Small config change
commit 9ca3adf6bd0cbdf093bc2a1c6116c7e51e89d2ab
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-16T14:51:47Z
STORM-2882: Shaded zookeeper
commit 89b11f68483d9d1940abd9054fdeac37f2739c5b
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-16T17:05:34Z
STORM-2882: jline
commit 2f023ef20e9af1d4d3bbe55dc67c2e9bf6e1df29
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-25T21:56:52Z
STORM-2882: Fixed some issues with tests with guava conflicts
commit 13315e8787e5597a912e0351c5ecdf447551e098
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-29T16:34:49Z
STORM-2882: commons-{lang,collections,io}
commit 2ed99008776494c58a33ada5cbdc1179e6703158
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-29T18:05:07Z
STORM-2882: jctools
commit 6fb2ce8e77b05c8faa143e496056295a1a7d16f5
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-29T18:57:28Z
STORM-2882: JAXB
commit 4f81520d756fdd2616a9a23f26af9a35bb5f846c
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-29T19:42:12Z
STORM-2882: json-simple
commit 169d1979b40f97ca35f39743f4c9964ef6ba3e4e
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-29T20:26:52Z
STORM-2882: snakeyaml
commit 857d4ed05ac4430b9d0fbe561ae8986b74d2cb83
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T12:27:10Z
STORM-2882: netty
commit a7d784afc521a01665fa494ee1a499038db8cee1
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T13:09:09Z
STORM-2882: jgrapht
commit 77cc82ade1baa752a0ac1f5872e46d8f20d41aaf
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T15:37:55Z
STORM-2882: thrift
commit 87109c6f70a0843b8940fea7fe93199fa14d1126
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T17:03:57Z
STORM-2882: removed dep declaration
commit ea60f13f96b3325889a7a6921b925beb41781a16
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T18:23:42Z
STORM-2882: commons-codec httpclient
commit 084c8589531be585dc8a920f7da4078e3a1723dd
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T19:09:15Z
STORM-2882: httpcore
commit 513dd9214ca746082791b17b576eccd3ab18f7a5
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-05-30T20:04:15Z
STORM-2882: sysout-over-slf4j
----
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2698
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
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-ganglia
metrics-graphite
minlog
objeneses
reflectasm
ring-cors - removed in 2.x
servlet-api-2.5 - removed by this patch
slf4j
storm-core - removed in 2.x
storm-rename-hack - removed in 2.x
The only dependency on the storm client classpath not in 1.2.1 is org.acplt:oncrpc:jar:1.0.7 because it was pulled in by a newer version of io.dropwizard.metrics:metrics-ganglia.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193103356
--- Diff: storm-server/src/main/java/org/apache/storm/testing/InProcessZookeeper.java ---
@@ -12,15 +12,16 @@
package org.apache.storm.testing;
-import java.util.List;
+import org.apache.storm.shade.org.apache.zookeeper.server.NIOServerCnxnFactory;
import org.apache.storm.zookeeper.Zookeeper;
-import org.apache.zookeeper.server.NIOServerCnxnFactory;
/**
* A local Zookeeper instance available for testing.
+ * ```java
--- End diff --
Neat, didn't occur to me to check :)
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by ptgoetz <gi...@git.apache.org>.
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 the pom files, checks in changes, tags, and pushes to ASF git. So if you want to test the release process you will need to revert those changes.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
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 it will call the shaded APIs directly.
The upside is that IDEs work grate because they are operating on the shaded APIs and don't have to worry about how shading works. The downside is that doing a full build is now a 2 step process. This is because of a "bug" in maven/shade were shading in a multi-module project results in down stream builds not seeing the shaded dependencies. I cannot really combine them into a single build step because the version of maven with this bug in it is now standard in most places, so our "fix" for 1.x is not really valid any more.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193084777
--- Diff: storm-client/src/jvm/org/apache/storm/container/cgroup/SystemOperation.java ---
@@ -54,8 +55,8 @@ public static String exec(String cmd) throws IOException {
Process process = new ProcessBuilder(new String[]{ "/bin/bash", "-c", cmd }).start();
try {
process.waitFor();
- String output = IOUtils.toString(process.getInputStream());
- String errorOutput = IOUtils.toString(process.getErrorStream());
+ String output = IOUtils.toString(process.getInputStream(), Charsets.UTF_8);
--- End diff --
No, I think UTF-8 makes sense. Wasn't aware it was the default for bash. I thought maybe https://docs.oracle.com/javase/7/docs/api/java/nio/charset/Charset.html#defaultCharset() would work, but I'm not sure that will match the encoding used by bash.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192433936
--- Diff: DEVELOPER.md ---
@@ -239,8 +239,13 @@ To pull in a merge request you should generally follow the command line instruct
# Build the code and run the tests
## Prerequisites
-First of all you need to make sure you are using maven 3.2.5 or below. There is a bug in later versions of maven as linked to from https://issues.apache.org/jira/browse/MSHADE-206 that
-cause shaded dependencies to not be packaged correctly. Also please be aware that because we are shading dependencies mvn dependency:tree will not always show the dependencies correctly.
+To work around a conflict between the shade plugin and maven https://issues.apache.org/jira/browse/MSHADE-206 the build is spit into 2 parts.
+First a set of dependencies are shaded and installed by running.
+
+```sh
+cd shaded-deps
+mvn clean install
+```
--- End diff --
A little reordering of the sentences:
To work around a conflict between the shade plugin and maven https://issues.apache.org/jira/browse/MSHADE-206, a set of dependencies needs to be shaded and installed by running:
```sh
cd shaded-deps
mvn clean install
```
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193089630
--- Diff: storm-core/pom.xml ---
@@ -193,10 +193,6 @@
<artifactId>curator-framework</artifactId>
<scope>compile</scope>
</dependency>
- <dependency>
--- End diff --
Great catch, I'll try to clean up storm-core's dependencies to get them to the minimal.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193104685
--- Diff: storm-core/pom.xml ---
@@ -193,10 +193,6 @@
<artifactId>curator-framework</artifactId>
<scope>compile</scope>
</dependency>
- <dependency>
--- End diff --
I'm primarily concerned about https://issues.apache.org/jira/browse/STORM-2978 popping back up.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
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 too, or clients will not be able to find their dependencies. Just not sure where that is documented, or what scripts there are that do it, if any.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193080453
--- Diff: storm-client/src/jvm/org/apache/storm/container/cgroup/SystemOperation.java ---
@@ -54,8 +55,8 @@ public static String exec(String cmd) throws IOException {
Process process = new ProcessBuilder(new String[]{ "/bin/bash", "-c", cmd }).start();
try {
process.waitFor();
- String output = IOUtils.toString(process.getInputStream());
- String errorOutput = IOUtils.toString(process.getErrorStream());
+ String output = IOUtils.toString(process.getInputStream(), Charsets.UTF_8);
--- End diff --
I can change it back if you want. I noticed that toString is deprecated if you don't provide a character set, so I picked UTF_8 as that is the java default. I agree that bash/posix can be configured to use different character sets, but I thought UTF_8 was fairly standard now.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192816102
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
--- End diff --
I think jaxb-api is missing from here. It was in the dependency list for storm-client.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
I addressed everything except for jaxb. Because it is for java9 I will download a java9 JDK and work on that to make sure it works there too, so it might be a little longer.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192822576
--- Diff: storm-client/test/jvm/org/apache/storm/security/auth/ClientAuthUtilsTest.java ---
@@ -172,59 +178,47 @@ public void makeDigestPayloadTest() throws NoSuchAlgorithmException {
public void invalidConfigResultsInIOException() throws RuntimeException {
HashMap<String, Object> conf = new HashMap<>();
conf.put("java.security.auth.login.config", "__FAKE_FILE__");
- Assert.assertNotNull(AuthUtils.GetConfiguration(conf));
+ Assert.assertNotNull(ClientAuthUtils.getConfiguration(conf));
}
@Test
public void validConfigResultsInNotNullConfigurationTest() throws IOException {
File file1 = folder.newFile("mockfile.txt");
HashMap<String, Object> conf = new HashMap<>();
conf.put("java.security.auth.login.config", file1.getAbsolutePath());
- Assert.assertNotNull(AuthUtils.GetConfiguration(conf));
- }
-
- @Test
- public void uiHttpCredentialsPluginTest() {
- Map<String, Object> conf = new HashMap<>();
- conf.put(
- Config.UI_HTTP_CREDS_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
- conf.put(
- Config.DRPC_HTTP_CREDS_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
- conf.put(
- Config.STORM_PRINCIPAL_TO_LOCAL_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
- conf.put(
- Config.STORM_GROUP_MAPPING_SERVICE_PROVIDER_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
-
- Assert.assertTrue(
- AuthUtils.GetUiHttpCredentialsPlugin(conf).getClass() == AuthUtilsTestMock.class);
- Assert.assertTrue(
- AuthUtils.GetDrpcHttpCredentialsPlugin(conf).getClass() == AuthUtilsTestMock.class);
- Assert.assertTrue(
- AuthUtils.GetPrincipalToLocalPlugin(conf).getClass() == AuthUtilsTestMock.class);
- Assert.assertTrue(
- AuthUtils.GetGroupMappingServiceProviderPlugin(conf).getClass() == AuthUtilsTestMock.class);
+ Assert.assertNotNull(ClientAuthUtils.getConfiguration(conf));
}
@Test(expected = RuntimeException.class)
public void updateSubjectWithNullThrowsTest() {
- AuthUtils.updateSubject(null, null, null);
+ ClientAuthUtils.updateSubject(null, null, null);
}
@Test(expected = RuntimeException.class)
public void updateSubjectWithNullAutosThrowsTest() {
- AuthUtils.updateSubject(new Subject(), null, null);
+ ClientAuthUtils.updateSubject(new Subject(), null, null);
}
@Test
public void updateSubjectWithNullAutosTest() {
AuthUtilsTestMock mock = Mockito.mock(AuthUtilsTestMock.class);
Collection<IAutoCredentials> autos = Arrays.asList(new IAutoCredentials[]{ mock });
Subject s = new Subject();
- AuthUtils.updateSubject(s, autos, null);
+ ClientAuthUtils.updateSubject(s, autos, null);
Mockito.verify(mock, Mockito.times(1)).updateSubject(s, null);
}
+
+ @Test
+ public void uiHttpCredentialsPluginTest() {
--- End diff --
Is this test correctly named?
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192818795
--- Diff: storm-client/src/jvm/org/apache/storm/container/cgroup/SystemOperation.java ---
@@ -54,8 +55,8 @@ public static String exec(String cmd) throws IOException {
Process process = new ProcessBuilder(new String[]{ "/bin/bash", "-c", cmd }).start();
try {
process.waitFor();
- String output = IOUtils.toString(process.getInputStream());
- String errorOutput = IOUtils.toString(process.getErrorStream());
+ String output = IOUtils.toString(process.getInputStream(), Charsets.UTF_8);
--- End diff --
Won't this break if the system's standard charset isn't UTF-8?
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193081199
--- Diff: storm-client/test/jvm/org/apache/storm/security/auth/ClientAuthUtilsTest.java ---
@@ -172,59 +178,47 @@ public void makeDigestPayloadTest() throws NoSuchAlgorithmException {
public void invalidConfigResultsInIOException() throws RuntimeException {
HashMap<String, Object> conf = new HashMap<>();
conf.put("java.security.auth.login.config", "__FAKE_FILE__");
- Assert.assertNotNull(AuthUtils.GetConfiguration(conf));
+ Assert.assertNotNull(ClientAuthUtils.getConfiguration(conf));
}
@Test
public void validConfigResultsInNotNullConfigurationTest() throws IOException {
File file1 = folder.newFile("mockfile.txt");
HashMap<String, Object> conf = new HashMap<>();
conf.put("java.security.auth.login.config", file1.getAbsolutePath());
- Assert.assertNotNull(AuthUtils.GetConfiguration(conf));
- }
-
- @Test
- public void uiHttpCredentialsPluginTest() {
- Map<String, Object> conf = new HashMap<>();
- conf.put(
- Config.UI_HTTP_CREDS_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
- conf.put(
- Config.DRPC_HTTP_CREDS_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
- conf.put(
- Config.STORM_PRINCIPAL_TO_LOCAL_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
- conf.put(
- Config.STORM_GROUP_MAPPING_SERVICE_PROVIDER_PLUGIN,
- "org.apache.storm.security.auth.AuthUtilsTestMock");
-
- Assert.assertTrue(
- AuthUtils.GetUiHttpCredentialsPlugin(conf).getClass() == AuthUtilsTestMock.class);
- Assert.assertTrue(
- AuthUtils.GetDrpcHttpCredentialsPlugin(conf).getClass() == AuthUtilsTestMock.class);
- Assert.assertTrue(
- AuthUtils.GetPrincipalToLocalPlugin(conf).getClass() == AuthUtilsTestMock.class);
- Assert.assertTrue(
- AuthUtils.GetGroupMappingServiceProviderPlugin(conf).getClass() == AuthUtilsTestMock.class);
+ Assert.assertNotNull(ClientAuthUtils.getConfiguration(conf));
}
@Test(expected = RuntimeException.class)
public void updateSubjectWithNullThrowsTest() {
- AuthUtils.updateSubject(null, null, null);
+ ClientAuthUtils.updateSubject(null, null, null);
}
@Test(expected = RuntimeException.class)
public void updateSubjectWithNullAutosThrowsTest() {
- AuthUtils.updateSubject(new Subject(), null, null);
+ ClientAuthUtils.updateSubject(new Subject(), null, null);
}
@Test
public void updateSubjectWithNullAutosTest() {
AuthUtilsTestMock mock = Mockito.mock(AuthUtilsTestMock.class);
Collection<IAutoCredentials> autos = Arrays.asList(new IAutoCredentials[]{ mock });
Subject s = new Subject();
- AuthUtils.updateSubject(s, autos, null);
+ ClientAuthUtils.updateSubject(s, autos, null);
Mockito.verify(mock, Mockito.times(1)).updateSubject(s, null);
}
+
+ @Test
+ public void uiHttpCredentialsPluginTest() {
--- End diff --
It used to be, but not any more. I'll fix it.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192823787
--- Diff: storm-server/src/main/java/org/apache/storm/testing/InProcessZookeeper.java ---
@@ -12,15 +12,16 @@
package org.apache.storm.testing;
-import java.util.List;
+import org.apache.storm.shade.org.apache.zookeeper.server.NIOServerCnxnFactory;
import org.apache.storm.zookeeper.Zookeeper;
-import org.apache.zookeeper.server.NIOServerCnxnFactory;
/**
* A local Zookeeper instance available for testing.
+ * ```java
--- End diff --
I don't think this will get formatted correctly unless markdown syntax is allowed in Javadoc?
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.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 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-framework</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-client</artifactId>
+ </dependency>
+ <!-- recipes are not used by the client, but storm server uses the same
--- End diff --
I'll try to clean it up, The curator recepies are not used by the client, but they are used by storm-server, but because of how we share initialization code between the client and the server we need all usage of curator to be shaded.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193078809
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
--- End diff --
After I moved the servlet-api to storm server everything compiled and ran just fine without jaxb-api, so I removed it. If there is some corner case using it that I didn't hit please let me know.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192434951
--- Diff: examples/storm-loadgen/pom.xml ---
@@ -67,6 +67,22 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
<groupId>commons-cli</groupId>
<artifactId>commons-cli</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </dependency>
+ <dependency>
--- End diff --
nit: space
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2698
Thanks for addressing the comments. Looks good to me. I think it makes sense to add shaded-deps as a module to the root pom though. It will likely address the release issue, and means we don't have to manually keep its version number in sync with the rest of Storm. Is there a reason to not have it listed in the root?
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193090616
--- Diff: storm-server/src/main/java/org/apache/storm/testing/InProcessZookeeper.java ---
@@ -12,15 +12,16 @@
package org.apache.storm.testing;
-import java.util.List;
+import org.apache.storm.shade.org.apache.zookeeper.server.NIOServerCnxnFactory;
import org.apache.storm.zookeeper.Zookeeper;
-import org.apache.zookeeper.server.NIOServerCnxnFactory;
/**
* A local Zookeeper instance available for testing.
+ * ```java
--- End diff --
https://issues.apache.org/jira/browse/STORM-1218 had #891 to add that support in.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2698
Looks great. +1.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192822955
--- Diff: storm-core/pom.xml ---
@@ -193,10 +193,6 @@
<artifactId>curator-framework</artifactId>
<scope>compile</scope>
</dependency>
- <dependency>
--- End diff --
Why is only Zookeeper being removed?
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
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 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-framework</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-client</artifactId>
+ </dependency>
+ <!-- recipes are not used by the client, but storm server uses the same
+ shaded curator instance so we need to include it here so we have a
+ single ZK connection per daemon. -->
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-recipes</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <exclusions>
+ <exclusion>
+ <!-- This is only used for interactive zookeeper shell, and not needed for the client-->
+ <groupId>jline</groupId>
+ <artifactId>jline</artifactId>
+ </exclusion>
+ <exclusion>
+ <!-- Annotations are not needed for runtime operation.-->
+ <groupId>org.apache.yetus</groupId>
+ <artifactId>audience-annotations</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>commons-io</groupId>
+ <artifactId>commons-io</artifactId>
+ <scope>compile</scope>
--- End diff --
+1 I tried to remove/cleanup the ones I copies and pasted, but I guess I missed one
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2698
Just so you don't spend too much time on it, neither HBase nor Cassandra work with Java 9 right now. I skipped those modules when testing it last year. Also I didn't get farther than building and running the tests with Java 9, so I can't say whether it works beyond that. At the time I was hoping to be able to upgrade HBase and Cassandra before too long and do more tests after that, but they are still working on Java 9 support.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
All I did was build/run the unit tests and this time I was able to get everything to pass. Oddly I was able to compile all of the code without jaxb-api but I could not run the tests...
I decided not to shade the API because I am not sure where the implementation is, and I would need to shade both.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192814411
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-framework</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-client</artifactId>
+ </dependency>
+ <!-- recipes are not used by the client, but storm server uses the same
--- End diff --
Not sure I understand what this means. Is it saying that storm-server uses recipes, and that's why this dependency is here?
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
@srdo I can add shaded-deps to the root pom, but it is going to potentially mask a lot of build issues, and is going to make IDEs confused.
maven-3.2.6 and above added in a cache for dependencies in the pom.xml. The shade plugin tries to modify those dependencies on the fly and the maven cache wins.
https://issues.apache.org/jira/browse/MSHADE-206
What this means is that if shaded-deps is a part of the root pom.xml, all of the code built by maven is going to have both the shaded and the non-shaded version on the classpath when it compiles and when unit tests run, but only the shaded version will be on the classpath in production.
Similarly for IDEs that don't understand the shading plugin they will get confused and will likely not see the shaded dependencies so all the code I changed will start to show up as errors, while anyone who uses it will write code against the non-shaded deps, which because of MSHADE-206, will compile under maven, but won't run in production.
In older releases we told everyone to use an older version of maven that is not impacted by this, but that is getting harder and harder to do, especially with recent security disclosures.
If you really want me to I can add it in under a profile that would be on by default, but we could turn off when we do the check-in builds and when being imported into an IDE.
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r193080442
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
--- End diff --
It's there for Java 9+ compatibility https://issues.apache.org/jira/browse/STORM-2800.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
@srdo It might work to make them optional. I am not a maven expert when it comes to that..., or really just about anything with maven beyond setting up a basic build. I am happy to try it out, as it is a real pain. I will also try out using a profile to control when/how it is built, so hold off checking any of this in until I have tested them.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2698
@srdo your suggestion to mark the dependencies as optional in the shaded pom was a great one. I have it working now where shaded-deps is built by default, maven does not include any of the unwanted dependencies on the classpath while building or testing, and when setting up and IDE there is a profile that can be used to disable shaded-deps so the IDE is happy. What is more IntelliJ has that off by default so if you just import storm it will do the right thing out of the box.
I also updated the travis scripts to explicitly build/install shaded-deps first and then run the rest of the build with shaded-deps off, so it we know nothing is sneaking in there on accident.
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2698
@revans2 If I'm understanding the problem correctly, the bug is that if shaded-deps is in the same reactor as the other modules, Maven will read the "normal" pom, rather than the dependency reduced pom, and put all the shaded-deps dependencies onto the classpath of the other modules?
Would it work to make all the dependencies declared in shaded-deps optional (i.e. non-transitive)?
---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2698
Thanks @ptgoetz
---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2698#discussion_r192814731
--- Diff: shaded-deps/pom.xml ---
@@ -0,0 +1,272 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <groupId>org.apache.storm</groupId>
+ <artifactId>shaded-deps</artifactId>
+ <packaging>jar</packaging>
+ <name>Shaded Deps for Storm Client</name>
+ <description>Shaded version of dependencies used only for internal storm code.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-framework</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-client</artifactId>
+ </dependency>
+ <!-- recipes are not used by the client, but storm server uses the same
+ shaded curator instance so we need to include it here so we have a
+ single ZK connection per daemon. -->
+ <dependency>
+ <groupId>org.apache.curator</groupId>
+ <artifactId>curator-recipes</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <exclusions>
+ <exclusion>
+ <!-- This is only used for interactive zookeeper shell, and not needed for the client-->
+ <groupId>jline</groupId>
+ <artifactId>jline</artifactId>
+ </exclusion>
+ <exclusion>
+ <!-- Annotations are not needed for runtime operation.-->
+ <groupId>org.apache.yetus</groupId>
+ <artifactId>audience-annotations</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>commons-io</groupId>
+ <artifactId>commons-io</artifactId>
+ <scope>compile</scope>
--- End diff --
Nit: I think compile scope is the default
---