Copilot commented on code in PR #6656:
URL: https://github.com/apache/hive/pull/6656#discussion_r3820906012
##########
service/src/java/org/apache/hive/http/LlapServlet.java:
##########
@@ -100,6 +100,8 @@ public void doGet(HttpServletRequest request,
HttpServletResponse response) {
ExitCode ret =
driver.run(LlapStatusServiceCommandLine.parseArguments(new String[] {"-n",
clusterName}), 0);
if (ret == ExitCode.SUCCESS) {
driver.outputJson(writer);
+ } else {
+ response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}
Review Comment:
When `ret != ExitCode.SUCCESS` the servlet now returns HTTP 500, but it
still advertises a cacheable response (`Cache-Control: public, max-age=60`) set
earlier. Some proxies/CDNs may cache error responses when explicitly allowed,
causing transient failures to persist for up to 60s. Consider overriding
Cache-Control to `no-store` (or similar) for error responses.
##########
llap-server/src/java/org/apache/hadoop/hive/llap/cli/status/LlapStatusServiceDriver.java:
##########
@@ -71,7 +71,7 @@ public class LlapStatusServiceDriver {
private static final Logger CONSOLE_LOGGER =
LoggerFactory.getLogger("LlapStatusServiceDriverConsole");
private static final EnumSet<State> NO_YARN_SERVICE_INFO_STATES = EnumSet.of(
- State.APP_NOT_FOUND, State.COMPLETE, State.LAUNCHING);
+ State.COMPLETE, State.LAUNCHING);
Review Comment:
`State.APP_NOT_FOUND` was removed from `NO_YARN_SERVICE_INFO_STATES`, but
`processAppReport(null, ...)` sets state to `APP_NOT_FOUND` and returns
`ExitCode.SUCCESS`. With the current set, `run()` will continue on to query
YARN Service status/registry even though the application wasn't found, which
can turn a simple "app not found" state into a downstream client error (and in
the servlet path, a 500). Consider short-circuiting on `APP_NOT_FOUND` again to
avoid unnecessary YARN calls and preserve the intended state reporting.
##########
service/src/resources/hive-webapps/static/js/json.human.js:
##########
@@ -138,7 +138,9 @@
var boolOpt = options.bool;
container = document.createElement('div');
- if (boolOpt.showImage) {
+ if (boolOpt.showImage && boolOpt.img && boolOpt.img.true &&
boolOpt.img.false
+ && ('' + boolOpt.img.true).startsWith('/static/')
+ && ('' + boolOpt.img.false).startsWith('/static/')) {
Review Comment:
Using `String.prototype.startsWith` can break rendering in older browsers
(notably IE 11) that don't support it. Since this UI still has IE-specific
markup, prefer an ES5-safe prefix check (e.g., `indexOf(...) === 0`) for the
`/static/` validation.
This issue also appears on line 441 of the same file.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
