accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 23988: Use Dropwizard to create a proper REST monitor service
Date Wed, 30 Jul 2014 02:18:28 GMT


> On July 28, 2014, 5: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.

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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23988/#review48869
-----------------------------------------------------------


On July 28, 2014, 4: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, 4: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message