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 <[email protected]>님이 작성: > 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 < > [email protected]> 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 > [email protected]<mailto:[email protected]> 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 <[email protected] > <mailto:[email protected]>> wrote: > > Those look reasonable to me. > > > - Bobby > > > On Monday, July 31, 2017, 2:22:47 AM CDT, Jungtaek Lim <[email protected] > <mailto:[email protected]>> 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 <[email protected]<mailto: > [email protected]>>님이 작성: > > 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 < > [email protected]<mailto:[email protected]>> 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 <[email protected]>님이 작성: > > 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 <[email protected]>님이 > 작성: > > 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 < > [email protected]> 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 <[email protected]>님이 작성: > > 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 <[email protected]>님이 > 작성: > > 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 < > [email protected]> 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 <[email protected]>님이 작성: > > 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 <[email protected]>님이 > 작성: > > +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 < > [email protected]> > 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) > > > > >
