Copilot commented on code in PR #6514:
URL: https://github.com/apache/hive/pull/6514#discussion_r3361478572
##########
common/src/java/org/apache/hive/http/HttpServer.java:
##########
@@ -360,6 +366,9 @@ public void start() throws Exception {
}
public void stop() throws Exception {
+ if (this.keystoreChangeMonitor != null) {
+ this.keystoreChangeMonitor.cancel();
+ }
Review Comment:
In stop(), the monitor Timer is cancelled but the field is left non-null.
Clearing it after cancellation avoids accidental double-cancel attempts and
helps GC if stop() is called more than once (e.g., during shutdown hooks).
##########
common/src/java/org/apache/hive/http/HttpServer.java:
##########
@@ -706,6 +720,36 @@ ServerConnector createAndAddChannelConnector(int
queueSize, Builder b) {
return connector;
}
+ @VisibleForTesting
+ void setKeystoreChangeMonitor(Timer monitor) {
+ keystoreChangeMonitor = monitor;
+ }
+
+ @VisibleForTesting
+ Timer createKeystoreChangeMonitor(long reloadInterval, String keyStorePath,
+ SslContextFactory sslContextFactory) {
+ LOG.info("Starting SSL Certificates Store Monitor. reload interval: {}ms,
keyStorePath: {}", reloadInterval, keyStorePath);
+ Timer timer = new Timer("SSL Certificates Store Monitor", true);
+ //
+ // The Jetty SSLContextFactory provides a 'reload' method which will
reload both
+ // truststore and keystore certificates.
+ //
+ timer.schedule(new FileMonitoringTimerTask(
+ Paths.get(keyStorePath),
+ path -> {
+ LOG.info("Reloading certificates from store keystore " +
keyStorePath);
Review Comment:
This LOG.info uses string concatenation, unlike other logging in this class
which uses SLF4J parameterized messages. Using placeholders avoids unnecessary
string building and keeps logging consistent.
##########
common/src/test/org/apache/hive/http/TestHttpServer.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hive.http;
+
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.FileTime;
+import java.util.Timer;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.withSettings;
+
+/**
+ * Tests for the SSL keystore auto-reload feature wired in via
+ * {@code HttpServer#makeConfigurationChangeMonitor} and the surrounding
+ * {@code configurationChangeMonitor} field. See HiveConf
+ * {@code hive.server2.webui.keystore.reload.interval}.
+ */
+public class TestHttpServer {
+
+ private Path keystore;
+ private Timer timer;
+
+ @Before
+ public void setUp() throws Exception {
+ keystore = Files.createTempFile("test-keystore-", ".jks");
+ Files.write(keystore, "initial-content".getBytes());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (timer != null) {
+ timer.cancel();
+ }
+ if (keystore != null) {
+ Files.deleteIfExists(keystore);
+ }
+ }
+
+ /**
+ * The reload-interval ConfVar defaults to 60s so the feature is on out of
the box.
+ * Wiring in {@code createAndAddChannelConnector} treats <= 0 as "disabled".
+ */
+ @Test
+ public void testReloadIntervalConfDefaultIs60Seconds() {
+ HiveConf conf = new HiveConf();
+ long ms = conf.getTimeVar(
+ ConfVars.HIVE_SERVER2_WEBUI_SSL_KEYSTORE_RELOAD_INTERVAL,
TimeUnit.MILLISECONDS);
+ assertEquals(60_000L, ms);
+ }
+
+ /**
+ * When the watched keystore file is modified, the scheduled
+ * {@code FileMonitoringTimerTask} must invoke
+ * {@code SslContextFactory#reload}.
+ */
+ @Test(timeout = 10_000)
+ public void testMonitorReloadsSslContextOnKeystoreModification() throws
Exception {
+ SslContextFactory sslContextFactory = mock(SslContextFactory.class);
+ CountDownLatch reloadCalled = new CountDownLatch(1);
+ doAnswer(invocation -> {
+ reloadCalled.countDown();
+ return null;
+ }).when(sslContextFactory).reload(any());
+
+ timer = invokeMakeMonitor(100L, keystore.toString(), sslContextFactory);
+
+ // Bump mtime to guarantee a detected change (FileMonitoringTimerTask
compares mtimes).
+ Files.setLastModifiedTime(keystore,
FileTime.fromMillis(System.currentTimeMillis() + 5_000));
+
+ assertTrue("SslContextFactory#reload was not called within 5s of keystore
mtime change",
+ reloadCalled.await(5, TimeUnit.SECONDS));
+ verify(sslContextFactory, atLeastOnce()).reload(any());
+ }
+
+ /**
+ * Reload failures must be swallowed so a transient bad keystore can't take
HS2 down;
+ * the next mtime change should still trigger another reload attempt.
+ */
+ @Test(timeout = 10_000)
+ public void testMonitorSurvivesReloadException() throws Exception {
+ SslContextFactory sslContextFactory = mock(SslContextFactory.class);
+ CountDownLatch reloadCalled = new CountDownLatch(2);
+ doAnswer(invocation -> {
+ reloadCalled.countDown();
+ throw new RuntimeException("simulated keystore reload failure");
+ }).when(sslContextFactory).reload(any());
+
+ timer = invokeMakeMonitor(100L, keystore.toString(), sslContextFactory);
+
+ Files.setLastModifiedTime(keystore,
FileTime.fromMillis(System.currentTimeMillis() + 5_000));
+ Thread.sleep(300);
+ Files.setLastModifiedTime(keystore,
FileTime.fromMillis(System.currentTimeMillis() + 10_000));
+
+ assertTrue("Monitor should keep firing reload attempts even after
exceptions",
+ reloadCalled.await(5, TimeUnit.SECONDS));
+ }
+
+ /**
+ * {@code stop()} must cancel the monitor Timer when one was installed,
+ * so the daemon thread does not outlive HS2.
+ */
+ @Test
+ public void testStopCancelsConfigurationChangeMonitor() throws Exception {
+ HttpServer server = mock(HttpServer.class,
withSettings().defaultAnswer(CALLS_REAL_METHODS));
+
+ // Track whether cancel() was invoked on the installed timer.
+ boolean[] cancelled = {false};
+ Timer installed = new Timer("test-monitor", true) {
+ @Override
+ public void cancel() {
+ cancelled[0] = true;
+ super.cancel();
+ }
+ };
+ server.setKeystoreChangeMonitor(installed);
+
+ // stop() also calls webServer.stop(); webServer is null on a mock, so we
expect
+ // a NullPointerException after the cancel path runs.
+ try {
+ server.stop();
+ } catch (NullPointerException expected) {
+ // intentionally ignored — we only assert the monitor was cancelled
+ }
+ assertTrue("Timer#cancel should have been invoked from stop()",
cancelled[0]);
+ }
+
+ /**
+ * No monitor installed → stop() must not blow up trying to cancel a missing
Timer.
+ * (Mockito skips field initializers, so we re-establish the production
default
+ * {@code Optional.empty()} on the mock before exercising stop().)
+ */
+ @Test
+ public void testStopWithoutMonitorDoesNotThrowFromCancelPath() throws
Exception {
+ HttpServer server = mock(HttpServer.class,
withSettings().defaultAnswer(CALLS_REAL_METHODS));
+ server.setKeystoreChangeMonitor(null);
+ assertNull("keystoreChangeMonitor should be empty for this case",
server.keystoreChangeMonitor);
+
+ try {
+ server.stop();
+ } catch (NullPointerException expectedFromWebServerStop) {
+ // ok — the monitor branch must not have thrown before reaching
webServer.stop()
+ }
Review Comment:
The comment here refers to a production default of Optional.empty(), but
keystoreChangeMonitor is a plain Timer field (null when absent). Also, this
test currently expects a NullPointerException from webServer.stop() due to
mocking; it would be more robust to stub webServer and assert stop() completes
without throwing.
##########
common/src/test/org/apache/hive/http/TestHttpServer.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hive.http;
+
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.FileTime;
+import java.util.Timer;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.withSettings;
+
+/**
+ * Tests for the SSL keystore auto-reload feature wired in via
+ * {@code HttpServer#makeConfigurationChangeMonitor} and the surrounding
+ * {@code configurationChangeMonitor} field. See HiveConf
+ * {@code hive.server2.webui.keystore.reload.interval}.
+ */
+public class TestHttpServer {
+
+ private Path keystore;
+ private Timer timer;
+
+ @Before
+ public void setUp() throws Exception {
+ keystore = Files.createTempFile("test-keystore-", ".jks");
+ Files.write(keystore, "initial-content".getBytes());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (timer != null) {
+ timer.cancel();
+ }
+ if (keystore != null) {
+ Files.deleteIfExists(keystore);
+ }
+ }
+
+ /**
+ * The reload-interval ConfVar defaults to 60s so the feature is on out of
the box.
+ * Wiring in {@code createAndAddChannelConnector} treats <= 0 as "disabled".
+ */
+ @Test
+ public void testReloadIntervalConfDefaultIs60Seconds() {
+ HiveConf conf = new HiveConf();
+ long ms = conf.getTimeVar(
+ ConfVars.HIVE_SERVER2_WEBUI_SSL_KEYSTORE_RELOAD_INTERVAL,
TimeUnit.MILLISECONDS);
+ assertEquals(60_000L, ms);
+ }
+
+ /**
+ * When the watched keystore file is modified, the scheduled
+ * {@code FileMonitoringTimerTask} must invoke
+ * {@code SslContextFactory#reload}.
+ */
+ @Test(timeout = 10_000)
+ public void testMonitorReloadsSslContextOnKeystoreModification() throws
Exception {
+ SslContextFactory sslContextFactory = mock(SslContextFactory.class);
+ CountDownLatch reloadCalled = new CountDownLatch(1);
+ doAnswer(invocation -> {
+ reloadCalled.countDown();
+ return null;
+ }).when(sslContextFactory).reload(any());
+
+ timer = invokeMakeMonitor(100L, keystore.toString(), sslContextFactory);
+
+ // Bump mtime to guarantee a detected change (FileMonitoringTimerTask
compares mtimes).
+ Files.setLastModifiedTime(keystore,
FileTime.fromMillis(System.currentTimeMillis() + 5_000));
+
+ assertTrue("SslContextFactory#reload was not called within 5s of keystore
mtime change",
+ reloadCalled.await(5, TimeUnit.SECONDS));
+ verify(sslContextFactory, atLeastOnce()).reload(any());
+ }
+
+ /**
+ * Reload failures must be swallowed so a transient bad keystore can't take
HS2 down;
+ * the next mtime change should still trigger another reload attempt.
+ */
+ @Test(timeout = 10_000)
+ public void testMonitorSurvivesReloadException() throws Exception {
+ SslContextFactory sslContextFactory = mock(SslContextFactory.class);
+ CountDownLatch reloadCalled = new CountDownLatch(2);
+ doAnswer(invocation -> {
+ reloadCalled.countDown();
+ throw new RuntimeException("simulated keystore reload failure");
+ }).when(sslContextFactory).reload(any());
+
+ timer = invokeMakeMonitor(100L, keystore.toString(), sslContextFactory);
+
+ Files.setLastModifiedTime(keystore,
FileTime.fromMillis(System.currentTimeMillis() + 5_000));
+ Thread.sleep(300);
+ Files.setLastModifiedTime(keystore,
FileTime.fromMillis(System.currentTimeMillis() + 10_000));
+
+ assertTrue("Monitor should keep firing reload attempts even after
exceptions",
+ reloadCalled.await(5, TimeUnit.SECONDS));
+ }
+
+ /**
+ * {@code stop()} must cancel the monitor Timer when one was installed,
+ * so the daemon thread does not outlive HS2.
+ */
+ @Test
+ public void testStopCancelsConfigurationChangeMonitor() throws Exception {
+ HttpServer server = mock(HttpServer.class,
withSettings().defaultAnswer(CALLS_REAL_METHODS));
+
+ // Track whether cancel() was invoked on the installed timer.
+ boolean[] cancelled = {false};
+ Timer installed = new Timer("test-monitor", true) {
+ @Override
+ public void cancel() {
+ cancelled[0] = true;
+ super.cancel();
+ }
+ };
+ server.setKeystoreChangeMonitor(installed);
+
+ // stop() also calls webServer.stop(); webServer is null on a mock, so we
expect
+ // a NullPointerException after the cancel path runs.
+ try {
+ server.stop();
+ } catch (NullPointerException expected) {
+ // intentionally ignored — we only assert the monitor was cancelled
+ }
Review Comment:
This test currently relies on a NullPointerException from webServer.stop()
because the HttpServer instance is a CALLS_REAL_METHODS mock with null private
fields. That makes the test brittle (it can fail if stop() changes) and it
doesn’t verify the full stop() path. Consider injecting a stub webServer via
reflection so stop() completes without throwing.
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -3856,6 +3856,9 @@ public static enum ConfVars {
"SSL certificate keystore location for HiveServer2 WebUI."),
HIVE_SERVER2_WEBUI_SSL_KEYSTORE_PASSWORD("hive.server2.webui.keystore.password",
"",
"SSL certificate keystore password for HiveServer2 WebUI."),
+
HIVE_SERVER2_WEBUI_SSL_KEYSTORE_RELOAD_INTERVAL("hive.server2.webui.keystore.reload.interval",
"60s",
+ new TimeValidator(TimeUnit.MILLISECONDS),
+ "The refresh interval used to check if either of the keystore
certificate file has changed."),
Review Comment:
The ConfVar description is grammatically incorrect and doesn’t document the
disable behavior implemented in HttpServer (interval <= 0 disables the
monitor). Updating the wording avoids confusion for operators.
--
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]
