stoty commented on code in PR #6783:
URL: https://github.com/apache/hbase/pull/6783#discussion_r2019014571
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -609,23 +606,21 @@ private void initializeWebServer(String name, String
hostName, Configuration con
Preconditions.checkNotNull(webAppContext);
- HandlerCollection handlerCollection = new HandlerCollection();
+ Handler.Sequence handlers = new Handler.Sequence();
ContextHandlerCollection contexts = new ContextHandlerCollection();
RequestLog requestLog = HttpRequestLog.getRequestLog(name);
if (requestLog != null) {
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- handlerCollection.addHandler(requestLogHandler);
+ webServer.setRequestLog(requestLog);
Review Comment:
So loggers are not handlers now ?
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -670,8 +665,9 @@ private void initializeWebServer(String name, String
hostName, Configuration con
// Check if disable stack trace property is configured
if (!conf.getBoolean(HTTP_UI_SHOW_STACKTRACE_KEY, true)) {
// Disable stack traces for server errors in UI
- webServer.setErrorHandler(new ErrorHandler());
- webServer.getErrorHandler().setShowStacks(false);
+ ErrorHandler errorHanlder = new ErrorHandler();
Review Comment:
typo
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -1300,7 +1299,7 @@ public void stop() throws Exception {
li.listener.close();
} catch (Exception e) {
LOG.error("Error while stopping listener for webapp" +
webAppContext.getDisplayName(), e);
- exception = addMultiException(exception, e);
+ exception.addSuppressed(e);
Review Comment:
This looks buggy, _exception_ is null here.
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -1290,7 +1289,7 @@ void openListeners() throws Exception {
* stop the server
*/
public void stop() throws Exception {
- MultiException exception = null;
+ Exception exception = null;
Review Comment:
This is where we should initialize something.
##########
hbase-rest/src/main/java/org/apache/hadoop/hbase/rest/RESTServer.java:
##########
@@ -400,6 +404,20 @@ public synchronized void run() throws Exception {
server.start();
}
+ private static void setUriComplianceRules(HttpConfiguration httpConfig) {
+ // In Jetty 12, ambiguous path separators, suspicious path characters, and
ambiguous empty
+ // segments are considered violations of the URI specification and hence
are not allowed.
+ // Refer to
https://github.com/jetty/jetty.project/issues/11890#issuecomment-2156449534
+ // We must set a URI compliance to allow for this violation so that client
+ // requests are not automatically rejected. We have tests which rely on
this behavior.
+ // TODO Discuss Should we set below to UriCompliance.LEGACY instead of
cherry-picking?
Review Comment:
I think this is fine.
This signals that we may want to clean this up properly at some point.
##########
hbase-http/src/test/java/org/apache/hadoop/hbase/http/log/TestLogLevel.java:
##########
@@ -235,7 +235,10 @@ private boolean validateCommand(String[] args) {
* @throws Exception if unable to create or start a Jetty server
*/
private HttpServer createServer(String protocol, boolean isSpnego) throws
Exception {
- HttpServer.Builder builder = new HttpServer.Builder().setName("..")
+ // Changed to "" as ".." moves it a steps back in path because the path is
relative to the
Review Comment:
Strange thet it worked with jetty 9.
##########
hbase-rest/src/test/java/org/apache/hadoop/hbase/rest/TestGetAndPutResource.java:
##########
@@ -321,6 +321,8 @@ public void testLatestCellGetJSON() throws IOException {
@Test
public void testURLEncodedKey() throws IOException, JAXBException {
+ // Requires UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR
Review Comment:
I'm not sure if we CAN fix it, I haven't really checked, just in principle.
##########
hbase-shaded/hbase-shaded-check-invariants/src/test/resources/ensure-jars-have-correct-contents.sh:
##########
@@ -96,7 +96,10 @@ allowed_expr+="|^PropertyList-1.0.dtd$"
# Shaded jetty resources
allowed_expr+="|^about.html$"
allowed_expr+="|^jetty-dir.css$"
-
+# Required by jetty 12 on ee8
+allowed_expr="(|^javax/$)"
+# Required by jetty 12 on ee9
Review Comment:
nit: ee9+
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -670,8 +665,9 @@ private void initializeWebServer(String name, String
hostName, Configuration con
// Check if disable stack trace property is configured
if (!conf.getBoolean(HTTP_UI_SHOW_STACKTRACE_KEY, true)) {
// Disable stack traces for server errors in UI
- webServer.setErrorHandler(new ErrorHandler());
- webServer.getErrorHandler().setShowStacks(false);
+ ErrorHandler errorHanlder = new ErrorHandler();
+ errorHanlder.setShowStacks(false);
Review Comment:
would getting the handler and setting it not work ?
I'm fine with the change, just curious.
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -1213,14 +1212,14 @@ public void start() throws IOException {
} catch (IOException ex) {
LOG.info("HttpServer.start() threw a non Bind IOException", ex);
throw ex;
- } catch (MultiException ex) {
+ } catch (Exception ex) {
Review Comment:
So Jetty just throws Exception here ?
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -1311,28 +1310,41 @@ public void stop() throws Exception {
} catch (Exception e) {
LOG.error("Error while stopping web app context for webapp " +
webAppContext.getDisplayName(),
e);
- exception = addMultiException(exception, e);
+ exception.addSuppressed(e);
}
try {
webServer.stop();
} catch (Exception e) {
LOG.error("Error while stopping web server for webapp " +
webAppContext.getDisplayName(), e);
- exception = addMultiException(exception, e);
+ exception.addSuppressed(e);
Review Comment:
same
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -1311,28 +1310,41 @@ public void stop() throws Exception {
} catch (Exception e) {
LOG.error("Error while stopping web app context for webapp " +
webAppContext.getDisplayName(),
e);
- exception = addMultiException(exception, e);
+ exception.addSuppressed(e);
}
try {
webServer.stop();
} catch (Exception e) {
LOG.error("Error while stopping web server for webapp " +
webAppContext.getDisplayName(), e);
- exception = addMultiException(exception, e);
+ exception.addSuppressed(e);
}
if (exception != null) {
- exception.ifExceptionThrow();
+ ifExceptionThrow(exception);
}
}
- private MultiException addMultiException(MultiException exception, Exception
e) {
- if (exception == null) {
- exception = new MultiException();
+ /**
+ * Throw a {@link Throwable} as a checked {@link Exception} if it cannot be
thrown as unchecked.
+ * <p>
+ * <b>NOTE: This method is copied from Jetty-9
+ * @param throwable The {@link Throwable} to throw or null.
+ * @throws Error If the passed {@link Throwable} is an {@link Error}.
+ * @throws Exception Otherwise, if the passed {@link Throwable} is not null.
+ */
+ public static void ifExceptionThrow(Throwable throwable) throws Error,
Exception {
Review Comment:
Is this needed ?
We're throwing the Exception that we are creating explicitly, we know its
type exactly.
##########
hbase-rest/src/test/java/org/apache/hadoop/hbase/rest/TestGetAndPutResource.java:
##########
@@ -321,6 +321,8 @@ public void testLatestCellGetJSON() throws IOException {
@Test
public void testURLEncodedKey() throws IOException, JAXBException {
+ // Requires UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR
Review Comment:
Maybe we should just consider fixing these.
A new major version could be the opportunity to clean this up, even if we
break compatibility.
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -1311,28 +1310,41 @@ public void stop() throws Exception {
} catch (Exception e) {
LOG.error("Error while stopping web app context for webapp " +
webAppContext.getDisplayName(),
e);
- exception = addMultiException(exception, e);
+ exception.addSuppressed(e);
Review Comment:
same
##########
hbase-it/src/test/java/org/apache/hadoop/hbase/MockHttpApiRule.java:
##########
@@ -151,20 +157,25 @@ void clearRegistrations() {
}
@Override
- public void handle(final String target, final Request baseRequest,
- final HttpServletRequest request, final HttpServletResponse response) {
+ public boolean handle(Request request, Response response, Callback
callback) throws Exception {
+ String target = request.getHttpURI().getPath();
Review Comment:
Out of curiosity:
When would new API return false ?
##########
hbase-shaded/hbase-shaded-with-hadoop-check-invariants/src/test/resources/ensure-jars-have-correct-contents.sh:
##########
@@ -96,7 +96,10 @@ allowed_expr+="|^PropertyList-1.0.dtd$"
# Shaded jetty resources
allowed_expr+="|^about.html$"
allowed_expr+="|^jetty-dir.css$"
-
+# Required by jetty 12 on ee8
+allowed_expr="(|^javax/$)"
+# Required by jetty 12 on ee9
Review Comment:
nit ee9+
--
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]
