dlmarion commented on PR #5012: URL: https://github.com/apache/accumulo/pull/5012#issuecomment-2435066012
> Regarding Javalin, the existing monitor is well-established, and accomplishes a lot of goals that this demo of Javalin doesn't do (it handles serving JS/CSS resources from the classpath using the default servlet, for example, and automatically handles the serialization of POJO objects via the /rest endpoints in either JSON or XML using Jackson, with only JAX-RS annotations; it's also designed to be easily configured for TLS with client-auth certs, if desired, and handles a lot of other things). I think the switching cost of moving to Javelin are high, bring in a lot of new dependencies... on libraries written in a completely different programming language (Kotlin) that will be difficult for those unfamiliar to trace into for troubleshooting, if that becomes needed. I think I addressed this in my other comment, but since Javalin is a wrapper on top of Jetty, and it exposes all of the underlying Jetty controls, I don't think any of the points you raised regarding serving up static resources, TLS configuration, etc. are going to be an issue. I almost started with replacing the EmbeddedWebServer with Javalin completely, but then figured it would be better to keep them separate as it will be easier to remove the old monitor code once the new monitor is complete. The intention with this PR is to have two separate monitor (old and new) running off the same Monitor process until the new one is complete. All of this work could be done in a feature branch and *not* merged into `main`. @DomGarguilo's needs something that serves up the data to create the new UI. > It also isn't clear that the way it connects resources to endpoints is really all that much better or simpler than the JAX-RS specification that we get with Jersey annotations. I think it's simpler. With the current Monitor code you have to search for the class and method that has the matching path to understand which method is being called by the UI code. The way that the routes are built with Javalin, the mapping is centrally located in one place and can be made visible via the RouteOverview plugin to aid in debugging and development. > If we decide to expose the metrics directly from the servers, for general consumption by users who implement their own monitors, in order to get rid of our own monitor, a Javalin-based server might be a reasonable reference implementation. But, I don't think it should be part of these changes today. Exposing metrics directly from the servers for general consumption was not something that was discussed. This exposes metrics from the servers for the purpose of serving data to the Monitor. The MetricServiceHandler class in this PR currently exposes all of the Accumulo application metrics as it's unknown at this point which metrics will be needed by the Monitor. Once we know what that set is, we can further reduce the set of metrics being exposed by the new Thrift endpoint. > Regarding flatbuffers, I'd really like to understand why you did that, since it seems to me that it's not providing anything we don't already get with Thrift directly, and it adds extra layers and dependencies. I'm not sure I understand the intent. I think I addressed this in my other comment. > For the core change of exposing the metrics via Thrift, I was only able to review at a glance, because it was hard to see what was part of that core change, and what was part of the other things in this PR... but the overall mechanism seemed okay. However, it still depends on the assumption that we should put full metrics on the monitor (rather than just a small curated set of health/status items on a custom API, and leaving it to the user to collect metrics for analysis using a dedicated metrics-specific system, if they need that, rather than try to get them via our lightweight monitor). The intention is to just supply the metrics that are needed for the Monitor. Getting all of the metrics via a dedicated metrics system is the better and more complete solution. > Also, I was talking with @DomGarguilo earlier today and it occurred to me that it has happened a few times in the past that a user has come to us with an observation that the monitor is the only place where certain information exists, and as a solution, we've created public API methods to provide that information. So then I was wondering, instead of exposing a thrift RPC for metrics, that we use internally, maybe we should instead expose it as a public API, which the monitor can get directly from the AccumuloClient (it's own ServerContext). Then, anybody wanting to write their own monitor and expose the information differently, could just get the same information from the public API. This could be a step towards making the monitor an optional server that is merely another Accumulo client process. In the public API, we could also expose things that are not easily exposed in the metrics... like aggregated data per table or per tablet, which are not easily seen in the per-process m etrics alone. I think the intention here is to build the new Monitor off of exposed metric information. If there is something new that is needed, then we should add the metric for it. This approach means that the Monitor is not getting special data that can't be retrieved or calculated by just getting the metrics via some MeterRegistry. I don't think we should expose this via the public API in the AccumuloClient, I think that's the wrong approach. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
