> 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?
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 th e 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. > On July 28, 2014, 1:35 p.m., Christopher Tubbs wrote: > > assemble/bin/start-all.sh, lines 62-63 > > <https://reviews.apache.org/r/23988/diff/1/?file=643615#file643615line62> > > > > Since this component is optional, it seems like it should not be > > started automatically with the other standard services (the ones users have > > come to expect). > > Josh Elser wrote: > That depends. If the intent is still for the monitor (as we know it now) > to eventually use this service (make the monitor a consumer of this rest > service), I think it makes sense to just start the process and get people > used to it. > > Christopher Tubbs wrote: > Is it possible to spawn this as part of the monitor, rather than a > separate service, then? I don't mind a separate lib, but it seems a bit weird > to have a separate process. > > Josh Elser wrote: > Presently, no. Long term, the monitor process would go away and we'd move > the existing "monitor" into some templated views. But, that's out of the > scope of what I was initially trying to do here. I haven't entirely looked > into how to do that yet, either. Am I right in understanding that the separate lib/process is necessary, only because of DropWizard's launch magic? That this would go away entirely, and the extra classes/REST service could be in the monitor if the embedded Jetty was launched better in it's main method to look at Servlet 3.0 / JAX-RS annotations? (see below for link to example). - 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 > >
