> 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 > >
