You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Josh Elser <jo...@gmail.com> on 2014/07/28 18:55:10 UTC
Review Request 23988: Use Dropwizard to create a proper REST monitor service
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23988/
-----------------------------------------------------------
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
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Josh Elser <jo...@gmail.com>.
> On July 30, 2014, 7:02 p.m., Bill Havanki wrote:
> > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/MasterResource.java, line 56
> > <https://reviews.apache.org/r/23988/diff/1/?file=643638#file643638line56>
> >
> > To be more RESTful, consider returning an HTTP error response. (500, maybe a 404 or 204?)
Yeah, good call. I think I wrote this before I figured out the WebApplicationException(Status) stuff. There are probably more places to use response codes, too. Need to put more thought into the semantics behind the methods.
> On July 30, 2014, 7:02 p.m., Bill Havanki wrote:
> > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/health/AccumuloHealthCheck.java, lines 88-94
> > <https://reviews.apache.org/r/23988/diff/1/?file=643635#file643635line88>
> >
> > Is there a way to provide more detail about the failure to read the metadata table? I might like to know specifically that the check timed out vs. some other error.
Absolutely. I'll be making an umbrella JIRA later today (after chatting with Christopher). I'll try to address this too.
> On July 30, 2014, 7:02 p.m., Bill Havanki wrote:
> > server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/TableInformation.java, line 62
> > <https://reviews.apache.org/r/23988/diff/1/?file=643631#file643631line62>
> >
> > Use negative numbers to indicate no information, perhaps?
I think we really want these fields to be required. I need to look at the creator of this data (the remote side of the thrift call) that we're wrapping to see if there are ever cases in which this data might be absent.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23988/#review49138
-----------------------------------------------------------
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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23988/#review49138
-----------------------------------------------------------
server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/MonitorApplication.java
<https://reviews.apache.org/r/23988/#comment85981>
nit - whitespace blarg
server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/MonitorApplication.java
<https://reviews.apache.org/r/23988/#comment85978>
The hostname was already looked up before this method is called, so this line can be dropped.
server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/api/TableInformation.java
<https://reviews.apache.org/r/23988/#comment85982>
Use negative numbers to indicate no information, perhaps?
server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/health/AccumuloHealthCheck.java
<https://reviews.apache.org/r/23988/#comment85983>
Is there a way to provide more detail about the failure to read the metadata table? I might like to know specifically that the check timed out vs. some other error.
server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/MasterResource.java
<https://reviews.apache.org/r/23988/#comment85984>
THANKS :)
server/monitor-rest/src/main/java/org/apache/accumulo/monitor/rest/resources/MasterResource.java
<https://reviews.apache.org/r/23988/#comment85985>
To be more RESTful, consider returning an HTTP error response. (500, maybe a 404 or 204?)
- Bill Havanki
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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Josh Elser <jo...@gmail.com>.
> On July 28, 2014, 5: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.
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.
> 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.
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.
- 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Josh Elser <jo...@gmail.com>.
> 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Christopher Tubbs <ct...@apache.org>.
> 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Christopher Tubbs <ct...@apache.org>.
> 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.
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.
> On July 28, 2014, 1:35 p.m., Christopher Tubbs wrote:
> > pom.xml, line 129
> > <https://reviews.apache.org/r/23988/diff/1/?file=643620#file643620line129>
> >
> > Are we introducing incompatibilities that will make it harder for users to use newer versions of Jetty? Why the version drop?
>
> Josh Elser wrote:
> Dropwizard bundles this all for us, and thus, to use it, requires the drop to make it work. Users presently don't care what version of Jetty is used. The point of the library isn't to make a war file that can be deployed but to make a singular artifact that run the service. If you're referring to system integrators (like what you're doing for fedora), the point is still moot because you don't need jetty installed on the system elsewhere. The dependencies are encapsulated within the monitor-rest artifact.
The point isn't moot. Many distributions disallow bundled binaries (because by some definitions, such are not considered "open source" even if they were derived from open source). And, forcing this to be a dependency (using the shaded jar approach) is a non-starter for such requirements. The approach to deal with bundled binaries is to rebuild from source... but that can't be done if it depends on an old version which has been superceded.
That's one reason why one might care what version of Jetty is being used. Another (quite important reason) why one might care is because an older version might have security vulnerabilities or bugs that require a user to use a newer version. If they cannot use a newer version, because we depend on something that exists in an older version (which happens routinely anyway, but it's still good to avoid if possible), that makes it that much harder for a user to adopt the software.
For my packaging requirements in Fedora, it doesn't really matter, because I simply won't build the REST service if it requires a version not available. It's an optional component (as implemented), so it wouldn't affect me much to exclude it.
> 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).
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.
- 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Dave Marion <dl...@hotmail.com>.
> 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.
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.
- Dave
-----------------------------------------------------------
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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Josh Elser <jo...@gmail.com>.
> 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.
>
> 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.
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.
- 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Christopher Tubbs <ct...@apache.org>.
> 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Josh Elser <jo...@gmail.com>.
> On July 28, 2014, 5: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).
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.
> On July 28, 2014, 5:35 p.m., Christopher Tubbs wrote:
> > pom.xml, line 129
> > <https://reviews.apache.org/r/23988/diff/1/?file=643620#file643620line129>
> >
> > Are we introducing incompatibilities that will make it harder for users to use newer versions of Jetty? Why the version drop?
Dropwizard bundles this all for us, and thus, to use it, requires the drop to make it work. Users presently don't care what version of Jetty is used. The point of the library isn't to make a war file that can be deployed but to make a singular artifact that run the service. If you're referring to system integrators (like what you're doing for fedora), the point is still moot because you don't need jetty installed on the system elsewhere. The dependencies are encapsulated within the monitor-rest artifact.
> On July 28, 2014, 5:35 p.m., Christopher Tubbs wrote:
> > server/monitor-rest/pom.xml, lines 84-96
> > <https://reviews.apache.org/r/23988/diff/1/?file=643622#file643622line84>
> >
> > The rest server uses flot?
Probably just a copy-paste leftover from when I created the monitor-rest module.
> 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.
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.
- 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Dave Marion <dl...@hotmail.com>.
> 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.
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.
- Dave
-----------------------------------------------------------
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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Sean Busbey <se...@manvsbeard.com>.
> 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.
>
> 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.
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
-----------------------------------------------------------
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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Sean Busbey <se...@manvsbeard.com>.
> 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.
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?
- Sean
-----------------------------------------------------------
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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Josh Elser <jo...@gmail.com>.
> 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.
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).
- 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
>
>
Re: Review Request 23988: Use Dropwizard to create a proper REST monitor
service
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23988/#review48869
-----------------------------------------------------------
What's the advantage of using dropwizard? It seems that most everything used here is just basic JAX-RS stuff.
assemble/bin/start-all.sh
<https://reviews.apache.org/r/23988/#comment85618>
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).
pom.xml
<https://reviews.apache.org/r/23988/#comment85614>
Are we introducing incompatibilities that will make it harder for users to use newer versions of Jetty? Why the version drop?
pom.xml
<https://reviews.apache.org/r/23988/#comment85616>
Doesn't this create a dependency cycle? If log4j is the logger we're using, there's no reason to translate log4j to slf4j, only to go back to log4j again.
server/monitor-rest/pom.xml
<https://reviews.apache.org/r/23988/#comment85621>
Oh, I see you've excluded it. Good.
server/monitor-rest/pom.xml
<https://reviews.apache.org/r/23988/#comment85622>
The rest server uses flot?
server/monitor-rest/pom.xml
<https://reviews.apache.org/r/23988/#comment85624>
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.
- Christopher Tubbs
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
>
>