[GitHub] storm issue #2701: STORM-3091 don't allow workers to create heartbeat direct...
Github user agresch commented on the issue: https://github.com/apache/storm/pull/2701 @revans2 - fixed the javadoc ---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
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/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> +4.0.0 + + +storm +org.apache.storm +2.0.0-SNAPSHOT +.. + + +org.apache.storm +shaded-deps +jar +Shaded Deps for Storm Client +Shaded version of dependencies used only for internal storm code. + + + +com.google.guava +guava + + +org.apache.curator +curator-framework + + +org.apache.curator +curator-client + + + +org.apache.curator +curator-recipes + + +org.apache.zookeeper +zookeeper + + + +jline +jline + + + +org.apache.yetus +audience-annotations + + + + +commons-io +commons-io +compile --- End diff -- +1 I tried to remove/cleanup the ones I copies and pasted, but I guess I missed one ---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
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/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> +4.0.0 + + +storm +org.apache.storm +2.0.0-SNAPSHOT +.. + + +org.apache.storm +shaded-deps +jar +Shaded Deps for Storm Client +Shaded version of dependencies used only for internal storm code. + + + +com.google.guava +guava + + +org.apache.curator +curator-framework + + +org.apache.curator +curator-client + +
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
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 @@ + + + +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";> +4.0.0 + + +storm +org.apache.storm +2.0.0-SNAPSHOT +.. + + +org.apache.storm +shaded-deps +jar +Shaded Deps for Storm Client +Shaded version of dependencies used only for internal storm code. + + + --- 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
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
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 @@ + + + +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";> +4.0.0 + + +storm +org.apache.storm +2.0.0-SNAPSHOT +.. + + +org.apache.storm +shaded-deps +jar +Shaded Deps for Storm Client +Shaded version of dependencies used only for internal storm code. + + + --- End diff -- It's there for Java 9+ compatibility https://issues.apache.org/jira/browse/STORM-2800. ---
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
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 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 conf = new HashMap<>();
conf.put("java.security.auth.login.config",
file1.getAbsolutePath());
-Assert.assertNotNull(AuthUtils.GetConfiguration(conf));
-}
-
-@Test
-public void uiHttpCredentialsPluginTest() {
-Map 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 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
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
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 @@ curator-framework compile - --- 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
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. ---
[CVE-2018-1332] Apache Storm user impersonation vulnerability
CVE-2018-1332: Apache Storm user impersonation vulnerability Severity: Important Vendor: The Apache Software Foundation Versions Affected: Apache Storm 1.2.1 Apache Storm 1.1.2 Description: Apache Storm version 1.0.6 and earlier, 1.2.1 and earlier, and version 1.1.2 and earlier expose a vulnerability that could allow a user to impersonate another user when communicating with some Storm Daemons. Mitigation: 1.2.1 users should upgrade to version 1.2.2. 1.1.2 users should upgrade to version 1.1.3. 1.0.6 users should upgrade to version 1.1.3. Apache Storm 1.2.2 artifacts are available for immediate download here: http://www.us.apache.org/dist/storm/apache-storm-1.2.2/ Apache Storm 1.1.3 artifacts are available for immediate download here: http://www.us.apache.org/dist/storm/apache-storm-1.1.3/ Credit: This issue was discovered by Bobby Evans of the Apache Storm PMC References: http://storm.apache.org/2018/06/04/storm122-released.html http://storm.apache.org/2018/06/04/storm113-released.html P. Taylor Goetz
[CVE-2018-8008] Apache Storm arbitrary file write vulnerability
CVE-2018-8008: Apache Storm arbitrary file write vulnerability Severity: Important Vendor: The Apache Software Foundation Versions Affected: Apache Storm 1.2.1 Apache Storm 1.1.2 Description: Apache Storm version 1.0.6 and earlier, 1.2.1 and earlier, and version 1.1.2 and earlier expose an arbitrary file write vulnerability, that can be achieved using a specially crafted zip archive (affects other archives as well, bzip2, tar, xz, war, cpio, 7z), that holds path traversal filenames. So when the filename gets concatenated to the target extraction directory, the final path ends up outside of the target folder. Mitigation: 1.2.1 users should upgrade to version 1.2.2. 1.1.2 users should upgrade to version 1.1.3. 1.0.6 users should upgrade to version 1.1.3. Apache Storm 1.2.2 artifacts are available for immediate download here: http://www.us.apache.org/dist/storm/apache-storm-1.2.2/ Apache Storm 1.1.3 artifacts are available for immediate download here: http://www.us.apache.org/dist/storm/apache-storm-1.1.3/ Credit: This issue was discovered by Snyk Security Research Team References: http://storm.apache.org/2018/06/04/storm122-released.html http://storm.apache.org/2018/06/04/storm113-released.html P. Taylor Goetz
[GitHub] storm pull request #2698: STORM-2882: shade storm-client dependencies
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 pull request #2698: STORM-2882: shade storm-client dependencies
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 @@ curator-framework compile - --- 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
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 issue #2698: STORM-2882: shade storm-client dependencies
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
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 #2701: STORM-3091 don't allow workers to create heartbeat...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2701 ---
[GitHub] storm pull request #2702: STORM-3092: Refactor code to correct launch initia...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2702 ---
[GitHub] storm pull request #2703: [STORM-3094] : Added topology name validation at c...
GitHub user ManoharVanam opened a pull request: https://github.com/apache/storm/pull/2703 [STORM-3094] : Added topology name validation at client side Current Behavior : Execute topology with invalid topology name is throwing exception after uploading the jar. Improvement : Validating topology name at client side before uploading the jar. You can merge this pull request into a Git repository by running: $ git pull https://github.com/ManoharVanam/storm STORM-3094 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2703.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 #2703 commit 45a8f3218c96738af075e7e984239a88b34eff01 Author: Manohar Vanam Date: 2018-06-05T18:37:30Z [STORM-3094] : Added topology name validation at client side ---
[GitHub] storm pull request #2704: STORM-1038: Upgrade to Netty 4
GitHub user srdo opened a pull request: https://github.com/apache/storm/pull/2704 STORM-1038: Upgrade to Netty 4 This is a continuation of the work done at https://github.com/apache/storm/pull/728. ### Important changes: * Split MessageEncoder into multiple encoders depending on the message type, and made the stateless ones singletons * Moved worker context termination to after the transfer thread has shut down. Shutting the context down first looks racy to me, since the transfer thread uses it. * ChannelGroups automatically remove closed Channels, so Server/PacemakerServer doesn't bother removing closed channels manually anymore * Made some changes to TransferDrainer for readability. I didn't see any performance impact from them, and they aren't necessary to upgrade to Netty 4. Let me know if they should go in another PR. # Benchmarking I benchmarked on a single-node install. Results are accessible at https://drive.google.com/open?id=1OuYHusyshQbzx38q5jchj8W98WG6An0L. ## Speed of light Ran `./storm jar .\storm-perf-2.0.0-SNAPSHOT.jar org.apache.storm.perf.ConstSpoutIdBoltNullBoltTopo 600 .\ConstSpoutIdBoltNullBoltTopo.yaml` with the following config ``` spout.count : 1 bolt1.count : 1 # IdBolt instances bolt2.count : 1 # DevNullBolt instances topology.workers : 3 ``` This should force all messages to cross to another worker, which hopefully does a good job showing any decrease in throughput for the changed code. ## TVL I didn't put much thought into setting the number of workers, or component counts here, let me know if I should try rerunning with some other numbers. Ran `./storm jar .\storm-loadgen-2.0.0-SNAPSHOT.jar org.apache.storm.loadgen.ThroughputVsLatency --rate 9 --spouts 4 --splitters 4 --counters 4 --reporter 'tsv:test.csv' -c topology.workers=4 --test-time 10`. 90k tuples is more than my machine can handle, so this is mostly to demonstrate that Storm doesn't choke any worse than it did before. Also ran `./storm jar .\storm-loadgen-2.0.0-SNAPSHOT.jar org.apache.storm.loadgen.ThroughputVsLatency --rate 75000 --spouts 4 --splitters 4 --counters 4 --reporter 'tsv:test.csv' -c topology.workers=4 --test-time 10` 75k is right on the edge of what my computer can put through, so it shows what latencies look like compared to master. Performance looks pretty much the same to me pre- and post these changes. Let me know if there are other tests I should run to validate these changes. # To do at some point * ByteBufs are reference counted, so we should validate that we don't leak references when testing. Netty can tell us when a leak occurs, but I haven't set anything up to fail the build if these logs occur during testing yet https://netty.io/wiki/reference-counted-objects.html#leak-detection-levels. I did run some tests with the detection level set to paranoid, and didn't see any leaks. * Didn't really test Pacemaker You can merge this pull request into a Git repository by running: $ git pull https://github.com/srdo/storm STORM-1038-clean Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2704.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 #2704 commit 20db8c339cbabefab1fe4cccb74d68c87cc36264 Author: Hang Sun Date: 2018-05-11T13:57:47Z STORM-1038: Upgrade to Netty 4.x. See https://github.com/apache/storm/pull/728 for the original contribution. commit 2d0f96102e20ca1c89fb65d05a860456c9d10a96 Author: Stig Rohde Døssing Date: 2018-05-11T14:55:35Z STORM-1038: Upgrade to latest Netty commit 5fbe93339061c539ae20e06c7158f892ba4abb3c Author: Stig Rohde Døssing Date: 2018-06-05T14:32:06Z Refactor of transfer drainer ---
[GitHub] storm pull request #2705: STORM-3096 prevent race condition during topology ...
GitHub user agresch opened a pull request: https://github.com/apache/storm/pull/2705 STORM-3096 prevent race condition during topology submission My previous attempt at fixing this addressed the race in store.storedTopoIds(). But state.idsOfTopologiesWithPrivateWorkerKeys() will also return a topology as ready to clean up while the topology is being submitted. The fix is to delay deletion on all topologies found for all the state and store checks. I was able to reproduce the issue and validate the fix properly this time by forcing a Nimbus cleanup to be called as part of topology submission. You can merge this pull request into a Git repository by running: $ git pull https://github.com/agresch/storm agresch_blob2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2705.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 #2705 commit 8f60cefe4bd57f4d143f47e1b8862765abaad445 Author: Aaron Gresch Date: 2018-06-05T20:58:29Z STORM-3096 prevent race condition during topology submission ---
[GitHub] storm pull request #2704: STORM-1038: Upgrade to Netty 4
Github user danny0405 commented on a diff in the pull request: https://github.com/apache/storm/pull/2704#discussion_r193270487 --- Diff: pom.xml --- @@ -82,7 +82,7 @@ Committer - + --- End diff -- nit whitespace ---
[GitHub] storm pull request #2704: STORM-1038: Upgrade to Netty 4
Github user danny0405 commented on a diff in the pull request: https://github.com/apache/storm/pull/2704#discussion_r193270523 --- Diff: pom.xml --- @@ -109,7 +109,7 @@ Committer - + --- End diff -- nit whitespace ---
[GitHub] storm issue #2704: STORM-1038: Upgrade to Netty 4
Github user danny0405 commented on the issue: https://github.com/apache/storm/pull/2704 @srdo Great work, i will take my time to review this patch as soon as possible. ---
[GitHub] storm issue #2703: [STORM-3094] : Added topology name validation at client s...
Github user danny0405 commented on the issue: https://github.com/apache/storm/pull/2703 Sorry i don't think put the validation at Client side is a good idea, cause many kind of clients may interact with Master through thrift RPC, we must keep all the clients sync on this validation rule if we moved it to client side. Now the client kind i know: - java script - java - python Even if we all modified and sync the rule between all the client, we still can not prevent some private modification to break it. ---
