You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@storm.apache.org by Hugo Da Cruz Louro <hl...@hortonworks.com> on 2017/08/02 01:49:17 UTC

Re: [DISCUSS] Ideas for resolving storm-drpc-server compilation issue on IDE

I have been following this discussion thread as part of the storm-core-ui migration. I would like to bring up a couple of points:

* The names of the packages "storm-client" and "storm-server" are a bit misleading to me. Isn’t what we really mean here "storm-workers" and "storm-daemons” ? Even if not these names, we should pick names that as close as possible to the “physical system”.

* storm-client-misc
* I noticed that this module only has two classes [1]. They are currently used in the module storm-starter and nowhere else. If that is the case, we should just put the classes in the module storm-starter. The concern is if some users may be using them in their deployments. Do you know of any users using these classes? Perhaps we could poll users@storm.apache.org<ma...@storm.apache.org> and find out.

* the -misc extension is also very confusing to me. My first thought was that it was some sort of library dependency placeholder, or something like that. If at all possible, my suggestion would be for us to eliminate this module altogether.

* Since we Storm 2.0 is a major release, if we find out that not many users (maybe none) are using the classes [1] we could probably just put the classes HttpForwardingMetricsConsumer, HttpForwardingMetricsServer in storm-starter. As for the concern of breaking backwards compatibility, document a workaround using storm-starter.

Thanks,
Hugo

[1] - HttpForwardingMetricsConsumer, HttpForwardingMetricsServer

On Jul 31, 2017, at 6:51 AM, Bobby Evans <ev...@yahoo-inc.com.INVALID>> wrote:

Those look reasonable to me.

- Bobby

On Monday, July 31, 2017, 2:22:47 AM CDT, Jungtaek Lim <ka...@gmail.com>> wrote:

I agreed to minimize the target of shade & relocation artifacts minimal as
possible, but as we shaded almost everything (meaning non-relocation will
affect user experience) so may need to find exhaustive set of troublesome
artifacts and relocate at least them. (Maybe union of everyone's lists?)

For me Guava, HttpClient, Netty (maybe no need to shade for now if we don't
plan to upgrade to 4.x: package name differs) is in my list.

Would be better to initiate poll or discussion with separate thread?

- Jungtaek Lim (HeartSaVioR)

2017년 7월 20일 (목) 오전 2:27, Bobby Evans <ev...@yahoo-inc.com.invalid>>님이 작성:

I am fine with a separate project for relocated dependencies (or even just
separate packages, you do a maven install of them and not include them in
the IDE at all). Shading still has some drawbacks, but I think in a few
cases it makes since. I would prefer it if we picked a very small number
of dependencies that cause people issues and just shade those. Guava is
the big one that I worry about. Netty is a possibility and I think asm
would be another, but it is a transitive dependency so it would require us
with our own version of kryo exposing the kryo API but pulling in a shaded
asm.
The servlet-api concerns me, but it looks like it is tied to the
IHttpCredentialsPlugin which should move to the server package anyways.

The rest I am not concerned about, are things that are exposed to end
users, or are for test and not actually shipped.
$ mvn dependecy:tree...
[INFO] --- maven-dependency-plugin:2.8:tree (default-cli) @ storm-client
---
[INFO] org.apache.storm:storm-client:jar:2.0.0-SNAPSHOT
[INFO] +- uk.org.lidalia:sysout-over-slf4j:jar:1.0.2:compile
[INFO] +- org.slf4j:slf4j-api:jar:1.7.21:compile
[INFO] +- org.apache.logging.log4j:log4j-api:jar:2.8.2:compile
[INFO] +- org.apache.logging.log4j:log4j-core:jar:2.8.2:compile
[INFO] +- org.apache.logging.log4j:log4j-slf4j-impl:jar:2.8.2:compile
[INFO] +- org.slf4j:log4j-over-slf4j:jar:1.6.6:compile
[INFO] +- com.google.guava:guava:jar:16.0.1:compile
[INFO] +- org.apache.thrift:libthrift:jar:0.9.3:compile
[INFO] | \- org.apache.httpcomponents:httpcore:jar:4.4.1:compile
[INFO] +- commons-io:commons-io:jar:2.5:compile
[INFO] +- commons-lang:commons-lang:jar:2.5:compile
[INFO] +- commons-collections:commons-collections:jar:3.2.2:compile
[INFO] +- com.lmax:disruptor:jar:3.3.2:compile
[INFO] +- com.googlecode.json-simple:json-simple:jar:1.1:compile
[INFO] +- org.yaml:snakeyaml:jar:1.11:compile
[INFO] +- io.netty:netty:jar:3.9.0.Final:compile
[INFO] +- com.esotericsoftware:kryo:jar:3.0.3:compile
[INFO] | +- com.esotericsoftware:reflectasm:jar:1.10.1:compile
[INFO] | | \- org.ow2.asm:asm:jar:5.0.3:compile
[INFO] | +- com.esotericsoftware:minlog:jar:1.3.0:compile
[INFO] | \- org.objenesis:objenesis:jar:2.1:compile
[INFO] +- org.apache.zookeeper:zookeeper:jar:3.4.6:compile
[INFO] | \- jline:jline:jar:0.9.94:compile
[INFO] +- org.apache.curator:curator-framework:jar:2.12.0:compile
[INFO] +- org.jgrapht:jgrapht-core:jar:0.9.0:compile
[INFO] +- javax.servlet:servlet-api:jar:2.5:compile
[INFO] +- org.apache.httpcomponents:httpclient:jar:4.3.3:compile
[INFO] | +- commons-logging:commons-logging:jar:1.1.3:compile
[INFO] | \- commons-codec:commons-codec:jar:1.6:compile
[INFO] +- org.apache.curator:curator-client:jar:2.12.0:compile
[INFO] +- junit:junit:jar:4.11:test
[INFO] | \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] +- org.mockito:mockito-core:jar:1.9.5:test
[INFO] \- org.hamcrest:hamcrest-library:jar:1.3:test
- Bobby

On Wednesday, July 12, 2017, 9:45:43 AM CDT, Jungtaek Lim <
kabhwan@gmail.com<ma...@gmail.com>> wrote:

I'd like to bump on this again, since we have a few huge issues for Storm
2.0.0, and this issue is a kind of regression and effectively blocker.
(Please note that current master branch removes shading for some libraries
to make IDE happy.)

At that time I didn't consider option 2 as possible solution, but now Flink
is going with this option, and I can't find reason to not doing this.

* Repository: https://github.com/apache/flink-shaded
* Discussion thread:

http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-Changing-Flink-s-shading-model-td17419.html

Thought?

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 31일 (금) 오후 3:12, Jungtaek Lim <ka...@gmail.com>님이 작성:

Bobby,

I've worked on separating worker and daemon classpath.

- Issue: STORM-2441: Break down 'storm-core' to extract client (worker)
artifacts <http://issues.apache.org/jira/browse/STORM-2441>
- PR: https://github.com/apache/storm/pull/2034

I don't address your suggestion about "classpath selection" and "hiding
local mode". Please file issues if you would like to address.

Btw, I exclude artifacts from shade & relocation list so still need to
address dependency issue.

Folks,

any other ideas or opinions around dependency issue?

IMHO Option 2 is clearer but not sure where we can create a new git repo
(ASF git or even outside), and also it's not against LICENSEs to
repackage
shade & relocated artifacts to Maven.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 29일 (수) 오후 10:42, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
작성:

I am fine with those changes so long as we finish the separation of
worker
and daemon classpaths. Otherwise we have made some very big changes for
our end users that are going to have a hard time upgrading.
If all we support is the option to run an old worker version with a new
supervisor/nimbus I think that would be good enough, although I would
like
to see a full separation of the classpaths.

- Bobby

On Tuesday, March 28, 2017, 6:03:26 PM CDT, Jungtaek Lim <
kabhwan@gmail.com> wrote:Just FYI:
I've worked with minimal patch for 3, though I still don't like such
workaround:

https://github.com/HeartSaVioR/storm/commit/d3122faa7ae182915242b979beaac156f91fe3b2

It excludes 'libthrift', 'jetty', 'codahale metrics' from relocation
targets. I can see IDEA is OK to build the project, and Maven build
passing.

- Jungtaek Lim (HeartSaVioR)

2017년 3월 29일 (수) 오전 11:02, Jungtaek Lim <ka...@gmail.com>님이 작성:

Back to origin issue (before breaking down 'storm-core'), turned out
IntelliJ doesn't recognize relocated classes within project. That's why
build (via Maven) for master branch succeeds but IDEA compile doesn't.

There're some issues filed but no action has been made.
https://youtrack.jetbrains.com/issue/IDEA-93855
https://youtrack.jetbrains.com/issue/IDEA-126596

So suppose we have two modules A and B within project, and A relocates
L
to Lr.
B relies on A's method which returns a class of Lr or has parameters
for
a
class of LR, B needs to use Lr rather than L, and Lr is not recognized
from
B.

Moving 'storm-drpc-server' to 'storm-core' may help but it's not a nice
solution though. (think about why we add new module
'storm-drpc-server')
To minimize dependency for worker (which actually affects end users) we
should break down 'storm-core' and it will remain to be headache.

There seemed to be little workarounds.

1. Guide IDEA users to take hacky workaround.

Quoting
https://youtrack.jetbrains.com/issue/IDEA-93855#comment=27-1838157
:
"A hacky workaround is to make the module in intellij with the
dependency
depend on the jar explicitly in target/. This at least allows things to
compile and tests to run."

That is really bad and annoying, but we might have no choice when we
don't
want to take other workarounds.

2. Maintaining separate project for relocated dependencies.

This avoids contributors to take hacky workaround so good to go, but
maintaining relocated artifacts might be another headache, and I'm not
sure
ASF (or LICENSE of relocated targets) allows to do that.

3. Minimize (or remove) relocate targets and/or don't relocate
troublesome
targets.

For 'storm-drpc-server', there seems to be three troublesome targets:

- 'thrift'
- 'codahale metrics'
- 'jetty server' (We may be able to move this to 'storm-drpc-server'
when
another webapp port is done.)

If we are OK to give up relocating those things we might be OK for now.
We
may want to extend the list when we break down more modules from
'storm-core'.

Btw, IMHO relocating is not a good option. Elastic gives up shading
anything for 2.0. (
https://www.elastic.co/blog/to-shade-or-not-to-shade)
Someone might feel that it's a regression, but we need to decide to do
it
when it can provide better shape.

Please add ideas if you have any, and give your opinions about above
options.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 28일 (화) 오후 10:23, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
작성:

Sure I am happy to help out how I can. I really would like to spend
more
time on storm, but sadly work has shifted and my team got 2 new
projects
recently, but we have not increased the head count to cover it yet, so
I
am
swamped. But if you do need help with some of these let me know and
I'll
see what I can do in my spare time.

- Bobby

On Tuesday, March 28, 2017, 2:10:46 AM CDT, Jungtaek Lim <
kabhwan@gmail.com> wrote:Bobby,

I just tried to follow your suggestion and found it's less error-prone
compared to my approach, and has lots of benefits. (I am seeing the
great
chance to minimize dependencies for 'storm-client', say, Worker.)

Thanks for the suggestion. I'm working on this now. I'll mention you
when I
finish working this, or need your help.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 28일 (화) 오전 8:15, Jungtaek Lim <ka...@gmail.com>님이 작성:

I think we could also fix this issue for separating 'storm-core' and
'storm-webapp' (rename from 'storm-drpc-server'), since local cluster
doesn't need to have 'storm-webapp', DRPC server (local DRPC will still
be
in 'storm-core'), UI, Logviewer. That's what I'm working on, which
seems
to
require heavy efforts.

Your plan looks really promising, but in other perspective this plan is
even much harder to address.
Do you have time frame for working on this? If you can finish the work
in
time frame so that it can be included in 2.0.0, I'll just discard my
work
and move forward to port other things (logviewer, ui) first.

Regarding local mode, exposing local mode provides easy debug
functionality
with IDE, and hiding it takes away such functionality. We have
ConfigurableTopology for 2.0.0 which helps to remove ceremony code, so
exposing is not that bad.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 27일 (월) 오후 11:10, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
작성:

+1 for fixing dependency/IDE issues, but I am not sure it is as simple
as
what you describe.

The issue is that there is no clean way to get local mode without
pulling
in almost all of the daemons too. If we are going to go through the
pain
of separating them out, I would prefer to do it once and do it right.
I
am
happy to help out with this, as it is something I have been thinking
about
for a while, but just haven't found the time to tackle on my own.
First we need a good way to give a control to our users about the base
classpath of the worker, ideally the JVM version too. We have been
doing a
really good job with rolling upgrades and I think it would be great if
we
could have multiple versions of storm/JVM installed on the worker nodes
and
the end user can pick what JVM and what version of storm they want
their
worker to run with. We can argue over details of how that would work
later. The point is that it lets us make changes to the classpath in
very
drastic ways without breaking end users.

Second we need a better way to hide local mode. Every example we have
supports local mode which means we will ship a copy of the storm
daemons
in
each topology jar if we pull them out of the default classpath. We
need
to
be able to run existing topologies that do not have "local mode
support"
in
local mode. We should be able to make storm-submitter work, there are
already stubs for this kind of thing, but we may need to play around
with
DRPC and a few other APIs to make it transparent.

We then create new jars from the existing storm-core and
storm-drpc-server.

storm-client - Just what the client and worker needs. The only
external
dependencies are logging and possibly metrics.
storm-local - This would pull in local mode dependencies (almost
everything
in storm core). We might even make it a test jar.

storm-daemon - all of our daemon processes (most if not all shading
removed). We can subdivide this more if we want to.

storm-core would go away or just pull in storm-client.
The storm jar command would by default only pull in storm-client and
its
dependencies. If you wanted local mode you could add in a flag that
would
adjust the classpath, boot up a local mode cluster, change the client
to
transparently interact with that instead of a regular cluster, and jump
to
the end users main. There could also be an option to just include
everything on the classpath without the local mode cluster. Ideally if
we
include everything on the classpath with storm jar, that would also
add a
flag that would make the supervisor include everything on the classpath
when launching the worker.

- Bobby

On Monday, March 27, 2017, 12:11:44 AM CDT, Jungtaek Lim <
kabhwan@gmail.com>
wrote:Hi devs,

I took a first step of finalizing port work via resolving dependency
issue
with DRPC.

Here's what I'm giving a try:
- rename 'storm-drpc-server' to 'storm-webapp'
- remove 'storm-core' from 'storm-drpc-server'
-- 'storm-drpc-server' will have its own library directory or shaded
jar
- create 'storm-common' and extract all the things used for both
'storm-core' and 'storm-webapp'

It requires numerous files to be moved to, and huge code block should
be
moved / modified. A bit painful to work on.

Other approach would be separating 'storm-worker' (or 'storm-client')
and
'storm-daemon', and link to different libraries directory.
(Maybe we could make uber jar for 'storm-daemon'.)
This also requires similar work and maybe introduce more big effect to
users.

Other than above ideas I don't have any other ideas. We're shading
libraries which are both needed from 'storm-core' and
'storm-drpc-server'
which in turn makes known issue - able to build with maven but IDE
can't
compile 'storm-drpc-server' project.

Please share other ideas if you have one.

Thanks,
Jungtaek Lim (HeartSaVioR)

Re: [DISCUSS] Ideas for resolving storm-drpc-server compilation issue on IDE

Posted by Jungtaek Lim <ka...@gmail.com>.

I agree that storm-client-misc is named badly. I was no idea how to name,
because I extracted them only for removing http dependency from
storm-client. Still not sure which name fits best, so OK to have
STORM-2670. Regarding removing them, I don't want to do it unless we poll
and be sure users don't use these classes.

+1 to have STORM-2671. We provided this hacky thing more than 1 year, and
next major release would be best chance to remove it.

I think storm-client and storm-server are named correctly. Abstracting
whole things to client and server (and webapp) makes much sense to me, and
this is the pattern I can see from big and success projects. If we name
modules close to detail, we may become adding modules continuously.

Ideally we need to remove storm-core but we still leave something on that
module, not easy to remove it for now.

2017년 8월 2일 (수) 오후 11:28, Bobby Evans <ev...@yahoo-inc.com.invalid>님이 작성:

> Yes some of the names need to be improved and if you have suggestions
> before we release a 2.x release that would be great.
> storm-client, is actually the client.  We have included the worker
> classpath in here too because if the classpath is different for the client
> that serializes the topology from the classpath for the worker that
> deserializes it,  hard to debug problems start to show up.  This is the
> most critical part as this is what the user sees.  Everything else is off
> of the user's class path so we can "fix" the packages in minor releases of
> storm.  There are some things here too that we may want to move to
> storm-server, but I have not done a complete walk though.  we should before
> we do a 2.0.0 release.  Some beta or even alpha releases might be good.
>
> storm-client-misc is badly named.  It should probably be moved to external
> and be called something more like http-forwarding-metrics.  In fact I filed
> STORM-2670 for this.  If others have suggestions for a name please let me
> know.
>
> Now for the stuff that may not be ideal, but we can change around after a
> release.
>
> storm-rename-hack - STORM-2671 it should just go away.
>
> storm-core - this is mostly things that were left over as we moved other
> stuff around.  It has command line commands (can probably move to
> storm-server, or we could have a separate package just for them); Some
> pieces of the storm-ui (as you know); a few things in daemon that will
> probably go away with STORM-2671; and some testing code that still depends
> on clojure (as the clojure client pieces have moved off to other
> packages).  Once the tests have all migrated out of clojure that part can
> go away too.
>
> storm-server is supposed to be everything else.  Initially we split the
> http part off from the java part, and had a separate package for DRPC,
> because I was hopeful that I could find time to upgrade the http server in
> drpc to be newer and let us do async requests, but I never found the time
> to do it.
>
>
> - Bobby
>
>
> On Tuesday, August 1, 2017, 8:49:40 PM CDT, Hugo Da Cruz Louro <
> hlouro@hortonworks.com> wrote:
>
> I have been following this discussion thread as part of the storm-core-ui
> migration. I would like to bring up a couple of points:
>
> * The names of the packages "storm-client" and "storm-server" are a bit
> misleading to me. Isn’t what we really mean here "storm-workers" and
> "storm-daemons” ? Even if not these names, we should pick names that as
> close as possible to the “physical system”.
>
> * storm-client-misc
>   * I noticed that this module only has two classes [1]. They are
> currently used in the module storm-starter and nowhere else. If that is the
> case, we should just put the classes in the module storm-starter. The
> concern is if some users may be using them in their deployments. Do you
> know of any users using these classes? Perhaps we could poll
> users@storm.apache.org<ma...@storm.apache.org> and find out.
>
>   * the -misc extension is also very confusing to me. My first thought was
> that it was some sort of library dependency placeholder, or something like
> that. If at all possible, my suggestion would be for us to eliminate this
> module altogether.
>
>   * Since we Storm 2.0 is a major release, if we find out that not many
> users (maybe none) are using the classes [1] we could probably just put the
> classes HttpForwardingMetricsConsumer, HttpForwardingMetricsServer in
> storm-starter. As for the concern of breaking backwards compatibility,
> document a workaround using storm-starter.
>
> Thanks,
> Hugo
>
> [1] - HttpForwardingMetricsConsumer, HttpForwardingMetricsServer
>
>
> On Jul 31, 2017, at 6:51 AM, Bobby Evans <evans@yahoo-inc.com.INVALID
> <ma...@yahoo-inc.com.INVALID>> wrote:
>
> Those look reasonable to me.
>
>
> - Bobby
>
>
> On Monday, July 31, 2017, 2:22:47 AM CDT, Jungtaek Lim <kabhwan@gmail.com
> <ma...@gmail.com>> wrote:
>
> I agreed to minimize the target of shade & relocation artifacts minimal as
> possible, but as we shaded almost everything (meaning non-relocation will
> affect user experience) so may need to find exhaustive set of troublesome
> artifacts and relocate at least them. (Maybe union of everyone's lists?)
>
> For me Guava, HttpClient, Netty (maybe no need to shade for now if we don't
> plan to upgrade to 4.x: package name differs) is in my list.
>
> Would be better to initiate poll or discussion with separate thread?
>
> - Jungtaek Lim (HeartSaVioR)
>
> 2017년 7월 20일 (목) 오전 2:27, Bobby Evans <evans@yahoo-inc.com.invalid<mailto:
> evans@yahoo-inc.com.invalid>>님이 작성:
>
> I am fine with a separate project for relocated dependencies (or even just
> separate packages, you do a maven install of them and not include them in
> the IDE at all).  Shading still has some drawbacks, but I think in a few
> cases it makes since.  I would prefer it if we picked a very small number
> of dependencies that cause people issues and just shade those.  Guava is
> the big one that I worry about. Netty is a possibility and I think asm
> would be another, but it is a transitive dependency so it would require us
> with our own version of kryo exposing the kryo API but pulling in a shaded
> asm.
> The servlet-api concerns me, but it looks like it is tied to the
> IHttpCredentialsPlugin which should move to the server package anyways.
>
> The rest I am not concerned about, are things that are exposed to end
> users, or are for test and not actually shipped.
> $ mvn dependecy:tree...
> [INFO] --- maven-dependency-plugin:2.8:tree (default-cli) @ storm-client
> ---
> [INFO] org.apache.storm:storm-client:jar:2.0.0-SNAPSHOT
> [INFO] +- uk.org.lidalia:sysout-over-slf4j:jar:1.0.2:compile
> [INFO] +- org.slf4j:slf4j-api:jar:1.7.21:compile
> [INFO] +- org.apache.logging.log4j:log4j-api:jar:2.8.2:compile
> [INFO] +- org.apache.logging.log4j:log4j-core:jar:2.8.2:compile
> [INFO] +- org.apache.logging.log4j:log4j-slf4j-impl:jar:2.8.2:compile
> [INFO] +- org.slf4j:log4j-over-slf4j:jar:1.6.6:compile
> [INFO] +- com.google.guava:guava:jar:16.0.1:compile
> [INFO] +- org.apache.thrift:libthrift:jar:0.9.3:compile
> [INFO] |  \- org.apache.httpcomponents:httpcore:jar:4.4.1:compile
> [INFO] +- commons-io:commons-io:jar:2.5:compile
> [INFO] +- commons-lang:commons-lang:jar:2.5:compile
> [INFO] +- commons-collections:commons-collections:jar:3.2.2:compile
> [INFO] +- com.lmax:disruptor:jar:3.3.2:compile
> [INFO] +- com.googlecode.json-simple:json-simple:jar:1.1:compile
> [INFO] +- org.yaml:snakeyaml:jar:1.11:compile
> [INFO] +- io.netty:netty:jar:3.9.0.Final:compile
> [INFO] +- com.esotericsoftware:kryo:jar:3.0.3:compile
> [INFO] |  +- com.esotericsoftware:reflectasm:jar:1.10.1:compile
> [INFO] |  |  \- org.ow2.asm:asm:jar:5.0.3:compile
> [INFO] |  +- com.esotericsoftware:minlog:jar:1.3.0:compile
> [INFO] |  \- org.objenesis:objenesis:jar:2.1:compile
> [INFO] +- org.apache.zookeeper:zookeeper:jar:3.4.6:compile
> [INFO] |  \- jline:jline:jar:0.9.94:compile
> [INFO] +- org.apache.curator:curator-framework:jar:2.12.0:compile
> [INFO] +- org.jgrapht:jgrapht-core:jar:0.9.0:compile
> [INFO] +- javax.servlet:servlet-api:jar:2.5:compile
> [INFO] +- org.apache.httpcomponents:httpclient:jar:4.3.3:compile
> [INFO] |  +- commons-logging:commons-logging:jar:1.1.3:compile
> [INFO] |  \- commons-codec:commons-codec:jar:1.6:compile
> [INFO] +- org.apache.curator:curator-client:jar:2.12.0:compile
> [INFO] +- junit:junit:jar:4.11:test
> [INFO] |  \- org.hamcrest:hamcrest-core:jar:1.3:test
> [INFO] +- org.mockito:mockito-core:jar:1.9.5:test
> [INFO] \- org.hamcrest:hamcrest-library:jar:1.3:test
> - Bobby
>
>
> On Wednesday, July 12, 2017, 9:45:43 AM CDT, Jungtaek Lim <
> kabhwan@gmail.com<ma...@gmail.com>> wrote:
>
> I'd like to bump on this again, since we have a few huge issues for Storm
> 2.0.0, and this issue is a kind of regression and effectively blocker.
> (Please note that current master branch removes shading for some libraries
> to make IDE happy.)
>
> At that time I didn't consider option 2 as possible solution, but now Flink
> is going with this option, and I can't find reason to not doing this.
>
> * Repository: https://github.com/apache/flink-shaded
> * Discussion thread:
>
>
> http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-Changing-Flink-s-shading-model-td17419.html
>
> Thought?
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
> 2017년 3월 31일 (금) 오후 3:12, Jungtaek Lim <ka...@gmail.com>님이 작성:
>
> Bobby,
>
> I've worked on separating worker and daemon classpath.
>
> - Issue: STORM-2441: Break down 'storm-core' to extract client (worker)
> artifacts <http://issues.apache.org/jira/browse/STORM-2441>
> - PR: https://github.com/apache/storm/pull/2034
>
> I don't address your suggestion about "classpath selection" and "hiding
> local mode". Please file issues if you would like to address.
>
> Btw, I exclude artifacts from shade & relocation list so still need to
> address dependency issue.
>
> Folks,
>
> any other ideas or opinions around dependency issue?
>
> IMHO Option 2 is clearer but not sure where we can create a new git repo
> (ASF git or even outside), and also it's not against LICENSEs to
> repackage
> shade & relocated artifacts to Maven.
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
> 2017년 3월 29일 (수) 오후 10:42, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
> 작성:
>
> I am fine with those changes so long as we finish the separation of
> worker
> and daemon classpaths.  Otherwise we have made some very big changes for
> our end users that are going to have a hard time upgrading.
> If all we support is the option to run an old worker version with a new
> supervisor/nimbus I think that would be good enough, although I would
> like
> to see a full separation of the classpaths.
>
>
> - Bobby
>
> On Tuesday, March 28, 2017, 6:03:26 PM CDT, Jungtaek Lim <
> kabhwan@gmail.com> wrote:Just FYI:
> I've worked with minimal patch for 3, though I still don't like such
> workaround:
>
>
>
> https://github.com/HeartSaVioR/storm/commit/d3122faa7ae182915242b979beaac156f91fe3b2
>
> It excludes 'libthrift', 'jetty', 'codahale metrics' from relocation
> targets. I can see IDEA is OK to build the project, and Maven build
> passing.
>
> - Jungtaek Lim (HeartSaVioR)
>
> 2017년 3월 29일 (수) 오전 11:02, Jungtaek Lim <ka...@gmail.com>님이 작성:
>
> Back to origin issue (before breaking down 'storm-core'), turned out
> IntelliJ doesn't recognize relocated classes within project. That's why
> build (via Maven) for master branch succeeds but IDEA compile doesn't.
>
> There're some issues filed but no action has been made.
> https://youtrack.jetbrains.com/issue/IDEA-93855
> https://youtrack.jetbrains.com/issue/IDEA-126596
>
> So suppose we have two modules A and B within project, and A relocates
> L
> to Lr.
> B relies on A's method which returns a class of Lr or has parameters
> for
> a
> class of LR, B needs to use Lr rather than L, and Lr is not recognized
> from
> B.
>
> Moving 'storm-drpc-server' to 'storm-core' may help but it's not a nice
> solution though. (think about why we add new module
> 'storm-drpc-server')
> To minimize dependency for worker (which actually affects end users) we
> should break down 'storm-core' and it will remain to be headache.
>
> There seemed to be little workarounds.
>
> 1. Guide IDEA users to take hacky workaround.
>
> Quoting
> https://youtrack.jetbrains.com/issue/IDEA-93855#comment=27-1838157
> :
> "A hacky workaround is to make the module in intellij with the
> dependency
> depend on the jar explicitly in target/. This at least allows things to
> compile and tests to run."
>
> That is really bad and annoying, but we might have no choice when we
> don't
> want to take other workarounds.
>
> 2. Maintaining separate project for relocated dependencies.
>
> This avoids contributors to take hacky workaround so good to go, but
> maintaining relocated artifacts might be another headache, and I'm not
> sure
> ASF (or LICENSE of relocated targets) allows to do that.
>
> 3. Minimize (or remove) relocate targets and/or don't relocate
> troublesome
> targets.
>
> For 'storm-drpc-server', there seems to be three troublesome targets:
>
> - 'thrift'
> - 'codahale metrics'
> - 'jetty server' (We may be able to move this to 'storm-drpc-server'
> when
> another webapp port is done.)
>
> If we are OK to give up relocating those things we might be OK for now.
> We
> may want to extend the list when we break down more modules from
> 'storm-core'.
>
> Btw, IMHO relocating is not a good option. Elastic gives up shading
> anything for 2.0. (
> https://www.elastic.co/blog/to-shade-or-not-to-shade)
> Someone might feel that it's a regression, but we need to decide to do
> it
> when it can provide better shape.
>
> Please add ideas if you have any, and give your opinions about above
> options.
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
> 2017년 3월 28일 (화) 오후 10:23, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
> 작성:
>
> Sure I am happy to help out how I can.  I really would like to spend
> more
> time on storm, but sadly work has shifted and my team got 2 new
> projects
> recently, but we have not increased the head count to cover it yet, so
> I
> am
> swamped.  But if you do need help with some of these let me know and
> I'll
> see what I can do in my spare time.
>
>
> - Bobby
>
> On Tuesday, March 28, 2017, 2:10:46 AM CDT, Jungtaek Lim <
> kabhwan@gmail.com> wrote:Bobby,
>
> I just tried to follow your suggestion and found it's less error-prone
> compared to my approach, and has lots of benefits. (I am seeing the
> great
> chance to minimize dependencies for 'storm-client', say, Worker.)
>
> Thanks for the suggestion. I'm working on this now. I'll mention you
> when I
> finish working this, or need your help.
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
> 2017년 3월 28일 (화) 오전 8:15, Jungtaek Lim <ka...@gmail.com>님이 작성:
>
> I think we could also fix this issue for separating 'storm-core' and
> 'storm-webapp' (rename from 'storm-drpc-server'), since local cluster
> doesn't need to have 'storm-webapp', DRPC server (local DRPC will still
> be
> in 'storm-core'), UI, Logviewer. That's what I'm working on, which
> seems
> to
> require heavy efforts.
>
> Your plan looks really promising, but in other perspective this plan is
> even much harder to address.
> Do you have time frame for working on this? If you can finish the work
> in
> time frame so that it can be included in 2.0.0, I'll just discard my
> work
> and move forward to port other things (logviewer, ui) first.
>
> Regarding local mode, exposing local mode provides easy debug
> functionality
> with IDE, and hiding it takes away such functionality. We have
> ConfigurableTopology for 2.0.0 which helps to remove ceremony code, so
> exposing is not that bad.
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
> 2017년 3월 27일 (월) 오후 11:10, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
> 작성:
>
> +1 for fixing dependency/IDE issues, but I am not sure it is as simple
> as
> what you describe.
>
> The issue is that there is no clean way to get local mode without
> pulling
> in almost all of the daemons too.  If we are going to go through the
> pain
> of separating them out, I would prefer to do it once and do it right.
> I
> am
> happy to help out with this, as it is something I have been thinking
> about
> for a while, but just haven't found the time to tackle on my own.
> First we need a good way to give a control to our users about the base
> classpath of the worker, ideally the JVM version too.  We have been
> doing a
> really good job with rolling upgrades and I think it would be great if
> we
> could have multiple versions of storm/JVM installed on the worker nodes
> and
> the end user can pick what JVM and what version of storm they want
> their
> worker to run with.  We can argue over details of how that would work
> later. The point is that it lets us make changes to the classpath in
> very
> drastic ways without breaking end users.
>
> Second we need a better way to hide local mode.  Every example we have
> supports local mode which means we will ship a copy of the storm
> daemons
> in
> each topology jar if we pull them out of the default classpath.  We
> need
> to
> be able to run existing topologies that do not have "local mode
> support"
> in
> local mode.  We should be able to make storm-submitter work, there are
> already stubs for this kind of thing, but we may need to play around
> with
> DRPC and a few other APIs to make it transparent.
>
> We then create new jars from the existing storm-core and
> storm-drpc-server.
>
> storm-client - Just what the client and worker needs.  The only
> external
> dependencies are logging and possibly metrics.
> storm-local - This would pull in local mode dependencies (almost
> everything
> in storm core).  We might even make it a test jar.
>
> storm-daemon - all of our daemon processes (most if not all shading
> removed).  We can subdivide this more if we want to.
>
> storm-core would go away or just pull in storm-client.
> The storm jar command would by default only pull in storm-client and
> its
> dependencies.  If you wanted local mode you could add in a flag that
> would
> adjust the classpath, boot up a local mode cluster, change the client
> to
> transparently interact with that instead of a regular cluster, and jump
> to
> the end users main.  There could also be an option to just include
> everything on the classpath without the local mode cluster.  Ideally if
> we
> include everything on the classpath with storm jar, that would also
> add a
> flag that would make the supervisor include everything on the classpath
> when launching the worker.
>
>
> - Bobby
>
> On Monday, March 27, 2017, 12:11:44 AM CDT, Jungtaek Lim <
> kabhwan@gmail.com>
> wrote:Hi devs,
>
> I took a first step of finalizing port work via resolving dependency
> issue
> with DRPC.
>
> Here's what I'm giving a try:
> - rename 'storm-drpc-server' to 'storm-webapp'
> - remove 'storm-core' from 'storm-drpc-server'
> -- 'storm-drpc-server' will have its own library directory or shaded
> jar
> - create 'storm-common' and extract all the things used for both
> 'storm-core' and 'storm-webapp'
>
> It requires numerous files to be moved to, and huge code block should
> be
> moved / modified. A bit painful to work on.
>
> Other approach would be separating 'storm-worker' (or 'storm-client')
> and
> 'storm-daemon', and link to different libraries directory.
> (Maybe we could make uber jar for 'storm-daemon'.)
> This also requires similar work and maybe introduce more big effect to
> users.
>
> Other than above ideas I don't have any other ideas. We're shading
> libraries which are both needed from 'storm-core' and
> 'storm-drpc-server'
> which in turn makes known issue - able to build with maven but IDE
> can't
> compile 'storm-drpc-server' project.
>
> Please share other ideas if you have one.
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
>
>
>
>

Re: [DISCUSS] Ideas for resolving storm-drpc-server compilation issue on IDE

Posted by Bobby Evans <ev...@yahoo-inc.com.INVALID>.

Yes some of the names need to be improved and if you have suggestions before we release a 2.x release that would be great.
storm-client, is actually the client. We have included the worker classpath in here too because if the classpath is different for the client that serializes the topology from the classpath for the worker that deserializes it, hard to debug problems start to show up. This is the most critical part as this is what the user sees. Everything else is off of the user's class path so we can "fix" the packages in minor releases of storm. There are some things here too that we may want to move to storm-server, but I have not done a complete walk though. we should before we do a 2.0.0 release. Some beta or even alpha releases might be good.

storm-client-misc is badly named. It should probably be moved to external and be called something more like http-forwarding-metrics. In fact I filed STORM-2670 for this. If others have suggestions for a name please let me know.

Now for the stuff that may not be ideal, but we can change around after a release.

storm-rename-hack - STORM-2671 it should just go away.

storm-core - this is mostly things that were left over as we moved other stuff around. It has command line commands (can probably move to storm-server, or we could have a separate package just for them); Some pieces of the storm-ui (as you know); a few things in daemon that will probably go away with STORM-2671; and some testing code that still depends on clojure (as the clojure client pieces have moved off to other packages). Once the tests have all migrated out of clojure that part can go away too.

storm-server is supposed to be everything else. Initially we split the http part off from the java part, and had a separate package for DRPC, because I was hopeful that I could find time to upgrade the http server in drpc to be newer and let us do async requests, but I never found the time to do it.

- Bobby

On Tuesday, August 1, 2017, 8:49:40 PM CDT, Hugo Da Cruz Louro <hl...@hortonworks.com> wrote:

I have been following this discussion thread as part of the storm-core-ui migration. I would like to bring up a couple of points:

Thanks,
Hugo

[1] - HttpForwardingMetricsConsumer, HttpForwardingMetricsServer

On Jul 31, 2017, at 6:51 AM, Bobby Evans <ev...@yahoo-inc.com.INVALID>> wrote:

Those look reasonable to me.

- Bobby

On Monday, July 31, 2017, 2:22:47 AM CDT, Jungtaek Lim <ka...@gmail.com>> wrote:

For me Guava, HttpClient, Netty (maybe no need to shade for now if we don't
plan to upgrade to 4.x: package name differs) is in my list.

Would be better to initiate poll or discussion with separate thread?

- Jungtaek Lim (HeartSaVioR)

2017년 7월 20일 (목) 오전 2:27, Bobby Evans <ev...@yahoo-inc.com.invalid>>님이 작성:

On Wednesday, July 12, 2017, 9:45:43 AM CDT, Jungtaek Lim <
kabhwan@gmail.com<ma...@gmail.com>> wrote:

At that time I didn't consider option 2 as possible solution, but now Flink
is going with this option, and I can't find reason to not doing this.

* Repository: https://github.com/apache/flink-shaded
* Discussion thread:

http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-Changing-Flink-s-shading-model-td17419.html

Thought?

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 31일 (금) 오후 3:12, Jungtaek Lim <ka...@gmail.com>님이 작성:

Bobby,

I've worked on separating worker and daemon classpath.

- Issue: STORM-2441: Break down 'storm-core' to extract client (worker)
artifacts <http://issues.apache.org/jira/browse/STORM-2441>
- PR: https://github.com/apache/storm/pull/2034

I don't address your suggestion about "classpath selection" and "hiding
local mode". Please file issues if you would like to address.

Btw, I exclude artifacts from shade & relocation list so still need to
address dependency issue.

Folks,

any other ideas or opinions around dependency issue?

IMHO Option 2 is clearer but not sure where we can create a new git repo
(ASF git or even outside), and also it's not against LICENSEs to
repackage
shade & relocated artifacts to Maven.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 29일 (수) 오후 10:42, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
작성:

- Bobby

On Tuesday, March 28, 2017, 6:03:26 PM CDT, Jungtaek Lim <
kabhwan@gmail.com> wrote:Just FYI:
I've worked with minimal patch for 3, though I still don't like such
workaround:

https://github.com/HeartSaVioR/storm/commit/d3122faa7ae182915242b979beaac156f91fe3b2

It excludes 'libthrift', 'jetty', 'codahale metrics' from relocation
targets. I can see IDEA is OK to build the project, and Maven build
passing.

- Jungtaek Lim (HeartSaVioR)

2017년 3월 29일 (수) 오전 11:02, Jungtaek Lim <ka...@gmail.com>님이 작성:

There're some issues filed but no action has been made.
https://youtrack.jetbrains.com/issue/IDEA-93855
https://youtrack.jetbrains.com/issue/IDEA-126596

There seemed to be little workarounds.

1. Guide IDEA users to take hacky workaround.

That is really bad and annoying, but we might have no choice when we
don't
want to take other workarounds.

2. Maintaining separate project for relocated dependencies.

3. Minimize (or remove) relocate targets and/or don't relocate
troublesome
targets.

For 'storm-drpc-server', there seems to be three troublesome targets:

- 'thrift'
- 'codahale metrics'
- 'jetty server' (We may be able to move this to 'storm-drpc-server'
when
another webapp port is done.)

If we are OK to give up relocating those things we might be OK for now.
We
may want to extend the list when we break down more modules from
'storm-core'.

Please add ideas if you have any, and give your opinions about above
options.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 28일 (화) 오후 10:23, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
작성:

- Bobby

On Tuesday, March 28, 2017, 2:10:46 AM CDT, Jungtaek Lim <
kabhwan@gmail.com> wrote:Bobby,

Thanks for the suggestion. I'm working on this now. I'll mention you
when I
finish working this, or need your help.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 28일 (화) 오전 8:15, Jungtaek Lim <ka...@gmail.com>님이 작성:

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 3월 27일 (월) 오후 11:10, Bobby Evans <ev...@yahoo-inc.com.invalid>님이
작성:

+1 for fixing dependency/IDE issues, but I am not sure it is as simple
as
what you describe.

We then create new jars from the existing storm-core and
storm-drpc-server.

storm-daemon - all of our daemon processes (most if not all shading
removed). We can subdivide this more if we want to.

- Bobby

On Monday, March 27, 2017, 12:11:44 AM CDT, Jungtaek Lim <
kabhwan@gmail.com>
wrote:Hi devs,

I took a first step of finalizing port work via resolving dependency
issue
with DRPC.

It requires numerous files to be moved to, and huge code block should
be
moved / modified. A bit painful to work on.

Please share other ideas if you have one.

Thanks,
Jungtaek Lim (HeartSaVioR)