[GitHub] storm pull request #2551: [STORM-2940] Increase the CAPACITY value in LoadAw...

GitHub user Ethanlm opened a pull request: https://github.com/apache/storm/pull/2551 [STORM-2940] Increase the CAPACITY value in LoadAwareShuffleGrouping Explained in https://issues.apache.org/jira/browse/STORM-2940. This is to fix the problem in some extreme cases that

[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2548 @agresch I meant having metric field in AutoTGT so that it can be changed to Gauge in Codahale metric for Metrics V2, but it's OK if lambda works in Metrics V1. ---

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Hi Alexandre, Thanks for the review. You’re right, the javadoc/source jars should not be there, though it looks like this has been the case for a long time and would have affected previous releases. It looks like the problem was introduced in 1.1.0. Have you seen the same issue in other 1.1.x

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Hello Taylor, Yes same issue in Storm 1.1.0 : we have a production storm cluster based on this release, but then we were using our own Kafka spout, no we weren't impacted by this packaging issue. Could the extraneous files be cleaned up from 1.2.0-final? Best regards, Alexandre Vermeerbergen

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user srdo commented on the issue: https://github.com/apache/storm/pull/2550 +1 ---

Re: [VOTE] Release Apache Storm 1.2.0 (rc4)

+1 (binding) > source - verify file (signature, MD5, SHA) -- source, tar.gz : OK -- source, zip : OK - extract file -- source, tar.gz : OK -- source, zip : OK - diff-ing extracted files between tar.gz and zip : OK - build source with JDK 7 -- source, tar.gz : OK > binary - verify file

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user erikdw commented on the issue: https://github.com/apache/storm/pull/2550 To be clear, I'm largely aligned with what you're both saying. I might not understand what precise problem you face that makes not squashing such a problem, but I too would appreciate the aesthetic

[GitHub] storm pull request #2531: STORM-2898: Support for WorkerToken authentication

Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2531#discussion_r166762630 --- Diff: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java --- @@ -2254,14 +2262,13 @@ private void renewCredentials() throws Exception

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2550 @erikdw I agree that in this case where we are cherry-picking several commits it's very hard to avoid multiple commits, and probably we shouldn't even try to squash them because that will completely

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2550 > The squashing should be done by the creator of the PR and not by the committer that is going to merge the code. Sorry but I have been exposing disagreement for this and I still

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2550 Other than that I agree most of @hmcl proposed, which is along with the best practice for what other big projects are following. If we require (not optional) squashing commits, I don't think

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user erikdw commented on the issue: https://github.com/apache/storm/pull/2550 @hmcl @HeartSaVioR : perhaps we can take this offline? (I admit to being guilty of starting this discussion in these PRs.) I am not grasping the actual problems that you encounter that make

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2550 @erikdw @hmcl @ptgoetz First of all, I need to make this clear: I'm not claiming that this PR should be changed. I'm OK to merge this in as it is, and I'll do it once 24hrs policy is met.

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user erikdw commented on the issue: https://github.com/apache/storm/pull/2550 @hmcl : now we're going down the rathole that I knew existed if we didn't take this offline. 😄 Before I get to the question of using command-line `git` to see the related commits, I must

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2550 @ptgoetz, @srdo and I have been using the following approach for squashing the patches in storm-kafka-client, which I believe works well: - It is reasonable to submit a PR for review that has

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user erikdw commented on the issue: https://github.com/apache/storm/pull/2550 @HeartSaVioR : I just updated all the commit messages in-line with @hmcl's proposal. I also noticed that I missed the addition of the `serialVersionUID` to Fields.java so I put that in and rebased.

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user erikdw commented on the issue: https://github.com/apache/storm/pull/2550 > First of all, I need to make this clear: I'm not claiming that this PR should be changed. I'm OK to merge this in as it is, and I'll do it once 24hrs policy is met. Let me first add just a

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2550 We may also discuss about rebase (along with squash in this case) vs merge, and I'm in favor of rebase, because we have huge chance for PR to be reviewed and merged not immediately. In this case

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2550 @HeartSaVioR I understand your point of not putting the burden on the contributors. I didn't really think that it could be a big hurdle for contributors. However, taking that into consideration it

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2550 @erikdw I am not sure I follow when you wrote: > All commits of a PR are bunched together. They can all be referenced as a group in git revert or git cherry-pick. In the first git log

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2550 > Let me first add just a bit more info to my PR's commit messages per @hmcl's suggestions. OK sure. Please let me know when you finished. > Except that the format of the mails

[GitHub] storm pull request #2552: STORM-2941: Fix checkstyle issues in KafkaSpout

GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/2552 STORM-2941: Fix checkstyle issues in KafkaSpout You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm STORM-2941

[GitHub] storm pull request #2531: STORM-2898: Support for WorkerToken authentication

Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2531#discussion_r166784163 --- Diff: storm-client/src/jvm/org/apache/storm/cluster/IStormClusterState.java --- @@ -184,21 +184,21 @@ * daemons will cause it to fail.

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Thanks for addressing md5/sha issue, Taylor. Verified src.tar.gz and src.zip. Alexandre, thanks again for thoughtful verification of release. It much helped for us and I really appreciate it. Please keep up the good work. I feel what Alexandre reported is a blocker since it leads strange error

[GitHub] storm pull request #2531: STORM-2898: Support for WorkerToken authentication

Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2531#discussion_r166756346 --- Diff: storm-client/src/jvm/org/apache/storm/security/auth/ThriftServer.java --- @@ -30,60 +29,62 @@ public class ThriftServer {

[GitHub] storm pull request #2551: [STORM-2940] Dynamically set the CAPACITY value of...

Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2551#discussion_r166758132 --- Diff: storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java --- @@ -85,6 +85,7 @@ public void prepare(WorkerTopologyContext

Re: [DISCUSS] Replace storm-kafka-client on 1.1.x-branch / 1.0.x-branch with 1.x-branch

Update: I just merged the patch for 1.1.x-branch so release phase of Storm 1.1.2 can be restarted. Patch for 1.0.x-branch from Erik is available and got some +1s but waiting for 24hrs rule. 2018년 2월 7일 (수) 오전 5:03, Stig Rohde Døssing 님이 작성: > Took a look at backporting to

[GitHub] storm pull request #2531: STORM-2898: Support for WorkerToken authentication

Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2531#discussion_r166761674 --- Diff: storm-client/src/jvm/org/apache/storm/security/auth/workertoken/WorkerTokenAuthorizer.java --- @@ -0,0 +1,139 @@ +/** + * Licensed to the

[CANCELED] [VOTE] Release Apache Storm 1.2.0 (rc3)

Canceling to include fix for STORM-2942. -Taylor > On Feb 6, 2018, at 4:01 PM, P. Taylor Goetz wrote: > > This is a call to vote on releasing Apache Storm 1.2.0 (rc3) > > Full list of changes in this release: > >

[GitHub] storm pull request #2549: STORM-2936 Overwrite latest storm-kafka-client 1.x...

Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2549 ---

[GitHub] storm pull request #2551: [STORM-2940] Dynamically set the CAPACITY value of...

Github user Ethanlm commented on a diff in the pull request: https://github.com/apache/storm/pull/2551#discussion_r166755231 --- Diff: storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java --- @@ -85,6 +85,7 @@ public void prepare(WorkerTopologyContext

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Filed https://issues.apache.org/jira/browse/STORM-2943 for storm-kafka-monitor src/javadoc inclusion issue and assigned to Taylor. 2018년 2월 8일 (목) 오전 5:32, Jungtaek Lim 님이 작성: > Thanks for addressing md5/sha issue, Taylor. Verified src.tar.gz and > src.zip. > > Alexandre,

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

I agree. As a heads up, I’m going to commit the the following change for 1.x and 1.1.x: --- a/storm-dist/binary/src/main/assembly/binary.xml +++ b/storm-dist/binary/src/main/assembly/binary.xml @@ -313,7 +313,7 @@ ${project.basedir}/../../external/storm-kafka-monitor/target

[GitHub] storm pull request #2551: [STORM-2940] Dynamically set the CAPACITY value of...

Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2551#discussion_r166750716 --- Diff: storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java --- @@ -85,6 +85,7 @@ public void prepare(WorkerTopologyContext

[GitHub] storm pull request #2551: [STORM-2940] Dynamically set the CAPACITY value of...

Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2551#discussion_r166749574 --- Diff: storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java --- @@ -44,7 +44,7 @@ import org.slf4j.LoggerFactory;

[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires

Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2548 ---

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Hello Jungtaek, Reporting bugs is the least I can do to thank you guys for developing Storm :) I understand that there will be a RC4, but just FYI I have tested RC3 on a small Storm cluster with our 10+ topologies (small setup: 1 Nimbus VM, 1 Supervisor VM, 1 Zookeeper VM, Kafka Broker 1.0.0 and

[GitHub] storm pull request #2531: STORM-2898: Support for WorkerToken authentication

Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2531#discussion_r166762351 --- Diff: storm-client/src/jvm/org/apache/storm/security/auth/workertoken/WorkerTokenAuthorizer.java --- @@ -0,0 +1,139 @@ +/** + * Licensed to the

[GitHub] storm issue #2552: STORM-2941: Fix checkstyle issues in KafkaSpout

Github user srdo commented on the issue: https://github.com/apache/storm/pull/2552 My bad, thanks for fixing it. +1 ---

[VOTE] Release Apache Storm 1.2.0 (rc4)

This is a call to vote on releasing Apache Storm 1.2.0 (rc4) Note that the only difference between rc4 and rc3 is the fix for https://issues.apache.org/jira/browse/STORM-2942 . Full list of changes in this release:

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Yes. It will be fixed. Thanks again for pointing it out. -Taylor > On Feb 7, 2018, at 2:57 PM, Alexandre Vermeerbergen > wrote: > > Hello Taylor, > > Yes same issue in Storm 1.1.0 : we have a production storm cluster > based on this release, but then we were using

[GitHub] storm issue #2531: STORM-2898: Support for WorkerToken authentication

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2531 I think I have addressed the review comments. Please take a look again. If things look good I will rebase and squash before merging. ---

[GitHub] storm pull request #2546: STORM-2934 - fix startup ClassNotFoundException wh...

Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2546 ---

[GitHub] storm issue #2552: STORM-2941: Fix checkstyle issues in KafkaSpout

Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2552 @srdo Not a big deal. I just didn't see any other pull request for it, and it was just 2 lines so I did it. ---

[GitHub] storm issue #2518: STORM-2902: Some improvements for storm-rocketmq module

Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2518 @vongosling I'm aware of RocketMQ, just meant that we have unmanaged modules nowadays in Storm repo. since many of us are focusing core, and some of us are making Kafka connector possible

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2550 +1. Thanks @erikdw, it's really good. Thanks also for the discussion. Looking forward to continue it in the dev list, and hopefully agreeing on a good solution. ---

Re: [VOTE] Release Apache Storm 1.2.0 (rc4)

+1 (binding) src distribution - Retrieved source archive and verified files - Built the binary package from the above source archive bin distribution - Ran different topologies in local cluster - Created a 3 node cluster with worker slots. - Deployed few topologies - Checked various

[GitHub] storm pull request #2550: STORM-2937: Overwrite latest storm-kafka-client 1....

GitHub user erikdw opened a pull request: https://github.com/apache/storm/pull/2550 STORM-2937: Overwrite latest storm-kafka-client 1.x-branch into 1.0.x-branch Mimics @HeartSaVioR 's work in STORM-2936 and PR #2549, but applies back to the more divergent 1.0.x-branch. Big thanks

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Looks like missing md5/sha for apache-storm-1.2.0-src.tar.gz as well as apache-storm-1.2.0-src.zip. I'll continue verifying without that. 2018년 2월 7일 (수) 오전 6:01, P. Taylor Goetz 님이 작성: > This is a call to vote on releasing Apache Storm 1.2.0 (rc3) > > Full list of changes in

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

+1 (binding) > source - verify file (signature, MD5, SHA) -- source, tar.gz : OK -- source, zip : OK - extract file -- source, tar.gz : OK -- source, zip : OK - diff-ing extracted files between tar.gz and zip : OK - build source with JDK 7 -- source, tar.gz : OK - build source dist --

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

CORRECTION: didn't check md5/sha from source targz/zip since the files are not presented. Will check again once they are available. 2018년 2월 7일 (수) 오후 10:49, Jungtaek Lim 님이 작성: > +1 (binding) > > > source > > - verify file (signature, MD5, SHA) > -- source, tar.gz : OK > --

[GitHub] storm issue #2546: STORM-2934 - fix startup ClassNotFoundException when miss...

Github user agresch commented on the issue: https://github.com/apache/storm/pull/2546 squashed ---

[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires

Github user agresch commented on the issue: https://github.com/apache/storm/pull/2548 @HeartSaVioR - is this what you were requesting? ---

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Corrected. Thanks for pointing that out. -Taylor > On Feb 7, 2018, at 7:39 AM, Jungtaek Lim wrote: > > Looks like missing md5/sha for apache-storm-1.2.0-src.tar.gz as well as > apache-storm-1.2.0-src.zip. > > I'll continue verifying without that. > > 2018년 2월 7일 (수) 오전

[GitHub] storm issue #2550: STORM-2937: Overwrite latest storm-kafka-client 1.x-branc...

Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2550 +1 successfully built for me and all tests passed. As far as squashing commits my opinion has always been if we do squash, it should be done when merging. There are a lot of cases where

Re: [VOTE] Release Apache Storm 1.2.0 (rc3)

Hello All, I hate to be the one who always give bad news, but as a matter of facts, Storm 1.2.0 RC3 installation from binary artifacts (both apache-storm-1.2.0-src.tar.gz and apache-storm-1.2.0.zip) leads to "by default KO Kafka monitor" in Nimbus UI (which dirty exceptions in ui.log) Here's for