> On July 28, 2014, 1:35 p.m., Christopher Tubbs wrote: > > server/monitor-rest/pom.xml, lines 98-131 > > <https://reviews.apache.org/r/23988/diff/1/?file=643622#file643622line98> > > > > Please don't shade by default in the build. It creates a nightmare for > > pom dependency resolution. We should not be shipping shaded binary > > artifacts in a release, or deploying them to maven central in a release. > > Josh Elser wrote: > That's the whole point of Dropwizard. I'd recommend you read into it - > https://dropwizard.github.io/dropwizard/getting-started.html, specifically > https://dropwizard.github.io/dropwizard/getting-started.html#building-fat-jars. > > Dave Marion wrote: > Consumers of the rest service may need the client classes depending on > what they are doing in their application. In this case they will have to use > the shaded jar, as it contains the client classes, and it will also pull in > the jax-rs, jax-b, and jackson jars which may conflict with what their > application is doing. If we have a shaded jar on the server side, for ease of > classpath or whatever, then I think we want to create a client jar that only > contains the client classes. > > Josh Elser wrote: > That's valid. The pojos could be split into their own artifact (long > term, probably rolled into the long talked about 'accumulo-client' jar or > similar). > > Christopher Tubbs wrote: > Shading comes with a whole host of problems, some of which I mentioned > above, but also because anybody having a dependency on some POJO inside the > jar is going to have a huge nightmare with additional bundled stuff. > > If that's the only way to reasonable work with dropwizard, then I think > we should look at alternative REST frameworks, for something that can be more > easily baked in to the existing monitor packaging. Alternatively, we could do > a full separation and provide that service as a separate project or contrib, > which might actually help us focus on providing a standard API for > metrics/stats that any monitoring tool might be able to use, rather than > enriching the existing one. > > On the other hand, that link you provide doesn't really explain that we > need to build shaded jars... it just explains what shaded jars are and why > you might want to create them. In our case, I think non-shaded is preferred. > It fits better into our existing build and scripts, and doesn't come with all > the problems that shaded jars have. > > Josh Elser wrote: > Won't restate the POJO resolution as I already addressed that. > > The reason I like the recommendation of the shaded jar is because no one > on this project is a real "expert" on setting up Java web services. It gets > out of the way to provide some easy standards of just writing code. I do like > the technology as well (Jackson, Jetty, and Jersey), but boy do I hate trying > to actually set it up by hand for numerous reasons (primarily death by 1000 > configuration parameters). > > Removing the shade might not be *too* bad because we could hide the > dependencies necessary via the assembly pom and the shell scripts (which is > the argument that Dropwizard typically makes - you just need a single jar > that can be deployed with a simple `java -jar foo.jar server` and not having > to futz with the classpath). Having our own collection of scripts will > mitigate some of the pains that shading solves. I'll see if I can get > something working without shade some evening. > > Integration with metrics systems is outside of the scope of what this is > providing, but there is already integration with JMX as well as Ganglia. Both > of those are also unrelated to these changes. While yes, it may be nice if we > could integrate with some magical metrics library that gives us everything we > want, I've yet to find such a thing. Until then, it's pointless to keep an > unmaintainable collection of code just because it "would be nice" if such a > magical library existed. This diff is making improvements to Accumulo > providing metrics data in a consumable format. > > The splitting out of the existing servlet classes into data producers > (REST endpoints), we already make one step closer to easing integration with > other systems. Additionally, creating POJOs for the data being returned also > allows these integrations to more reliably use the data we produce. > > Sean Busbey wrote: > The primary reason to build a shaded jar is to provide a simpler > deployment model. From what I read, the point of dropwizard is to make > several design choices for you and streamline around them. The idea being you > have a single executable jar you can use to run a REST service. > > Josh, is DropWizard using the maven shade plugin? Can we configure it to > shade the dependencies to its own package space? > > Christopher, would having the fat jar as an additional artifact with > anything it bundles moved into its own package space to avoid conflicts be > sufficient to address your concerns? > > Christopher Tubbs wrote: > Josh: Personally, when it comes to packaging java, I'm a big fan of > delivering simple libs (jars, wars, etc.) and deferring all the launching to > outside of the jar (preferably using JPackage utilities for standardized > scripts in Linux, like Ant and other java packages do). It simplifies > packaging, improves portability, and is relatively easily understood by any > java user. Trying to simplify things by providing using a "do it all" > framework may help in some areas, but is going to come at a cost. In this > case, I think the build is going to be harder to maintain/troubleshoot, puts > more burden on developers, and makes decisions for downstream users while > taking away their choices. In general, it's just a bad idea to include > bundled libraries like shading does. It's bad enough I have to deal with the > bundled minimized JavaScript from flot! I don't want to add to the headache > that downstream packagers might have. We should make that easier, because > convenient downstream packaging is how the best projects grow/spread (If you don't buy that last claim, just consider how many people do 'yum install httpd mysql php' vs. install from upstream tarballs). > > I think most of my concerns are alleviated, though, so long as we remove > the shading (though I'm still concerned about the need to lower the > dependency version... due to some presumed incompatibility? but that's not > too terrible to deal with). Though, I'm not sure the point, if we do that. I > think the Servlet 3.0 annotations with embedded Jetty should alleviate a lot > of the configuration nightmare you're talking about, and the JAX-RS > annotations with Jersey/Jackson (@Path, @GET, @JSON, etc.) on POJOs are > pretty much the same as you have them here. I actually don't see a lot of > DropWizard-specific stuffs here. And, these benefits could be added to the > main monitor artifact today, without a separate service. (See > http://www.eclipse.org/jetty/documentation/current/using-annotations-embedded.html) > That would alleviate my concerns about shading, keep the existing service > launch method, and provide the simplified REST interface you're trying to > accomplish here without a separate jar. > > Sean: I'm not sure what you mean by "own package space", but in answer to > your question to Josh, DropWizard isn't using the maven-shade-plugin. We are > using it (in this patch) to package DropWizard with all the dependencies. > > Dave Marion wrote: > Regarding the shaded jar and the comments on the dropwizard site, it > might make sense from their perspective when your implementation of > dropwizard is the application, like when you are packaging up an ear or a > war. In this case dropwizard is a component of a larger application. The > shaded jar has caused a change to the start / stop scripts, it doesn't follow > the conventions that are already in place. > > In my experience shaded jars cause nothing but trouble. Sure, it gets you > up and running faster, but it will bite someone later. For example, the > general.classpaths property has been modified to exclude the shaded jar. What > happens if a vendor or user does not do that? Having said that, I don't know > that you have much of an option as from what I can tell dropwizard depends on > different versions of commons-lang, guava, and maybe others. I don't see > these dependencies being excluded, I wonder if they have an effect on the > effective pom. > > Josh Elser wrote: > While it's all nice and well to say that it's supposedly easy to do this, > I've yet to see a patch for anyone to do this. I, on the other hand, have > already done this here. I'm a little frustrated that, if you had strong > opinion on this, they weren't brought up when I started the patch, FWIW. If > you'd like to make the changes that you proposed with Servlet 3.0, jax-rs and > jersey/jackson, feel free to do so. I don't care to go down that road again. > > I disagree with you that forcing ourselves to use an external framework > to manage the "stuff we don't care about" is a negative. The point of a "do > it all" framework is just that: you don't have to do *anything*. I don't > agree with your assertion that it will be more of a burdern for developers, > as dropwizard is giving us packaging that "works". Also, I'm not about to > enter into any sort of argument on application distribution, shared library > linking, or linux package management in general. I don't see how using a > library that manages the underlying jax-rs/webserver libraries makes things > harder for ourselves. The packaging itself is meant to simplify the process > for us. > > Features that Dropwizard provides that are easily configured: application > health checks (we can define "is accumulo healthy" checks), SSL support > handled implicitly (provide a keystore), monitor metrics/timings, > cache-controls, custom authentication controls, and more. These are a few > examples I pulled from a brief scan over their user manual. > > What is your actual concern here? In general, this discussion has really > gone down a rabbit hole with little value coming out of it. I've already > stated that I'll look into removing the shaded artifact (despite a shaded > server jar is a server-side resource and not used by users; I also stated I'd > look into lifting the POJOs out). I think if you want to continue this, you > should move your concerns to the dev list. I can start it if you'd like. > > Josh Elser wrote: > Also, I did just verify that, as expected, there's nothing preventing a > non-shaded jar from being used. I've about doubled the size of the assembly > component descriptor, but that was expected as well. I need to figure out > classpath BS as well as logback/log4j/slf4j junk to fix the build, but I > figured I would share that I have naively killed the shade. > > While the shade included here does overpackage in a way that would likely > break us down the road (e.g. hadoop jars are getting shaded in right now), I > haven't convinced myself if manually enumerating each dependency from > dropwizard is any better. The maven-dependency-plugin seems to have some bugs > in pulling in the transitive dependencies for just dropwizard-core which > forced me to do this (http://jira.codehaus.org/browse/MASSEMBLY-357, I > think). Suggestions welcome to avoid component descriptor bloat. > > Sean Busbey wrote: > Christopher, the shade plugin lets you relocate classes to avoid > conflicts when downstream projects use your artifact, or if you otherwise > need to isolate your dependencies from other libraries / client apps. > > > http://maven.apache.org/plugins/maven-shade-plugin/examples/class-relocation.html > > I was referring to that capability when I said move them to their own > package space.
Sean: Ah, I see. It doesn't address all my concerns, but it does help with classpath conflicts. Josh: I agree and understand that it's definitely easier to critique than to offer a patch to change, but that's kind of the role of ReviewBoard, and what I've been trying to do here. I'd love to offer more help, but unfortunately, that's all I can offer at the moment. If I get a free moment, I most certainly will. I sympathize with your frustration and apologize for being part of the cause of it. However, I cannot anticipate having a future opinion about something that would have directed your efforts elsewhere, especially when there are so many aspects of the project going on at the same time. However, I don't think your effort was actually wasted. I think that we (either myself, you, or another volunteer, as time permits) may be able to port the POJOs over to the existing monitor, and we will have learned as a community (directly because of your efforts) whether or not DropWizard is a worthwhile solution for our needs. If you want to discuss anything on the dev list, that's fine w ith me... I was only responding here, because my views were specific to the implementation of the issue being put up for review, and not to the general idea of using a framework for REST services. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23988/#review48869 ----------------------------------------------------------- On July 28, 2014, 12:55 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23988/ > ----------------------------------------------------------- > > (Updated July 28, 2014, 12:55 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-3005 > https://issues.apache.org/jira/browse/ACCUMULO-3005 > > > Repository: accumulo > > > Description > ------- > > Creates a proper REST service using Dropwizard, with the intent to eventually > replace the existing monitor's "data" component. Copies most of the > functionality (sans the log-forwarding) into a standalone application. > Returns data as JSON and tries to separate logic into consumable pieces. > Still uses the Monitor class for most Thrift interactions. > > > Diffs > ----- > > assemble/bin/accumulo 727a4c8 > assemble/bin/start-all.sh cebbd8c > assemble/bin/stop-all.sh 4bf06c0 > assemble/bin/stop-server.sh 52696af > assemble/conf/templates/accumulo-site.xml 08c905b > assemble/pom.xml 89a3747 > pom.xml ba6693d > server/monitor-rest/.gitignore PRE-CREATION > server/monitor-rest/pom.xml PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/MonitorApplication.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/MonitorConfiguration.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/GarbageCollection.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/GarbageCollectorCycle.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/GarbageCollectorStatus.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/LogEvent.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/RecoveryStatusInformation.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/ReplicationInformation.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/TableInformation.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/TabletServerInformation.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/TabletServerTableInformation.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/TabletServerWithTableInformation.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/health/AccumuloHealthCheck.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/GarbageCollectorResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/LogResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/MasterResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/ProblemsResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/ReplicationResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/StatisticsOverTimeResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/StatisticsResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/TablesResource.java > PRE-CREATION > > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/TabletServerResource.java > PRE-CREATION > server/monitor-rest/src/main/resources/accumulo-monitor-rest.yaml > PRE-CREATION > server/monitor-rest/src/test/resources/log4j.properties PRE-CREATION > server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java > 268516c > > Diff: https://reviews.apache.org/r/23988/diff/ > > > Testing > ------- > > Tested against a single-node Accumulo instance. While this code is much > easier to test, I haven't written new unit tests yet. > > > Thanks, > > Josh Elser > >
