Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10998 )
Change subject: IMPALA-6857: Add Jvm pause/GC Monitor utility and expose JMX metrics ...................................................................... Patch Set 11: (2 comments) Thanks! I think the interface for the callbacks is a little bit weird. Specifically, for a given handler, we seem to put "Webserver::ENABLE_RAW_JSON_KEY = "__raw__"" into the JSON-y document so that the template can render it, but we could have done the switch without using a templating language. But that's neither here nor there with your change, so I think it's fine. I think we should maybe be using the RawUrlCallback here since I think we're never using the template. If you disagree, I'm fine to +2 this as is as well, but I want to hear your thoughts on the comments below. http://gerrit.cloudera.org:8080/#/c/10998/11/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/10998/11/be/src/util/default-path-handlers.cc@207 PS11, Line 207: // Parse the JSON string returned from fe. It's pretty weird that we're doing an extra round of parsing. I think it's harmless enough, but we should explain it more. Specifically, I think the issue is that our framework for showing nice HTML with these things requires this? The other approach would have been to use RawUrlCallback which returns a byte stream. Anyway, could you expand the comment here to indicate why we have to do this parsing? http://gerrit.cloudera.org:8080/#/c/10998/11/be/src/util/default-path-handlers.cc@261 PS11, Line 261: webserver->RegisterUrlCallback("/jmx", "raw_text.tmpl", JmxHandler); Do you think raw_text.tmpl is ever ever used? I think because we set the "__json__" key in the handler, the template will never be used, so this is really a RawCallback of sorts. -- To view, visit http://gerrit.cloudera.org:8080/10998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30d897b7e063846ad6d8f88243e2c04264da0341 Gerrit-Change-Number: 10998 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 07 Aug 2018 18:56:32 +0000 Gerrit-HasComments: Yes
