ctubbsii commented on code in PR #4982:
URL: https://github.com/apache/accumulo/pull/4982#discussion_r1803639220
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/EmbeddedWebServer.java:
##########
@@ -41,30 +45,35 @@ public class EmbeddedWebServer {
Property.MONITOR_SSL_TRUSTSTORE,
Property.MONITOR_SSL_TRUSTSTOREPASS);
private final Server server;
- private final ServerConnector connector;
private final ServletContextHandler handler;
private final boolean secure;
- public EmbeddedWebServer(Monitor monitor, int port) {
+ private int actualHttpPort;
+ private ServerConnector connector;
+
+ public EmbeddedWebServer(final Monitor monitor, int httpPort) {
server = new Server();
final AccumuloConfiguration conf = monitor.getContext().getConfiguration();
secure = requireForSecure.stream().map(conf::get).allMatch(s -> s != null
&& !s.isEmpty());
- connector = new ServerConnector(server, getConnectionFactories(conf,
secure));
- connector.setHost(monitor.getHostname());
- connector.setPort(port);
+ addConnectors(monitor, conf, httpPort, secure);
handler =
new ServletContextHandler(ServletContextHandler.SESSIONS |
ServletContextHandler.SECURITY);
handler.getSessionHandler().getSessionCookieConfig().setHttpOnly(true);
handler.setContextPath("/");
}
- private static AbstractConnectionFactory[]
getConnectionFactories(AccumuloConfiguration conf,
+ private void addConnectors(Monitor monitor, AccumuloConfiguration conf, int
httpPort,
boolean secure) {
- HttpConnectionFactory httpFactory = new HttpConnectionFactory();
+
+ boolean configureHttp2 = conf.getBoolean(Property.MONITOR_SUPPORT_HTTP2);
+
+ final HttpConfiguration httpConfig = new HttpConfiguration();
+ httpConfig.setSendServerVersion(false);
Review Comment:
This seems unrelated, and only hides the Jetty version. Is that something we
want to keep independently of this PR? What's the intention here?
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -868,6 +868,8 @@ public enum Property {
+ " The resources that are used by default can be seen in"
+ "
`accumulo/server/monitor/src/main/resources/templates/default.ftl`.",
"2.0.0"),
+ MONITOR_SUPPORT_HTTP2("monitor.http2.support", "false", PropertyType.BOOLEAN,
+ "If true will configure the Monitor web server to support http2
connections.", "3.1.0"),
Review Comment:
I don't think this needs to be configurable. All modern browsers support it.
If it improves our monitor's performance in any way, it should just be used
automatically when we use TLS.
For the technical reasons listed on
https://stackoverflow.com/a/46789195/196405 , I suggest not bothering to
support it when TLS is off.
--
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]
