charlesconnell commented on code in PR #6381:
URL: https://github.com/apache/hbase/pull/6381#discussion_r1864904498
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/io/TestFileChangeWatcher.java:
##########
@@ -86,69 +83,50 @@ public static void cleanupTempDir() {
@Test
public void testEnableCertFileReloading() throws IOException {
Configuration myConf = new Configuration();
- String sharedPath = "/tmp/foo.jks";
+ String sharedPath = File.createTempFile("foo",
"foo.jks").getAbsolutePath();
myConf.set(X509Util.TLS_CONFIG_KEYSTORE_LOCATION, sharedPath);
myConf.set(X509Util.TLS_CONFIG_TRUSTSTORE_LOCATION, sharedPath);
AtomicReference<FileChangeWatcher> keystoreWatcher = new
AtomicReference<>();
AtomicReference<FileChangeWatcher> truststoreWatcher = new
AtomicReference<>();
X509Util.enableCertFileReloading(myConf, keystoreWatcher,
truststoreWatcher, () -> {
});
assertNotNull(keystoreWatcher.get());
- assertThat(keystoreWatcher.get().getWatcherThreadName(),
Matchers.endsWith("-foo.jks"));
+ assertThat(keystoreWatcher.get().getWatcherThreadName(),
Matchers.endsWith("foo.jks"));
assertNull(truststoreWatcher.get());
keystoreWatcher.getAndSet(null).stop();
truststoreWatcher.set(null);
- String truststorePath = "/tmp/bar.jks";
+ String truststorePath = File.createTempFile("bar",
"bar.jks").getAbsolutePath();
myConf.set(X509Util.TLS_CONFIG_TRUSTSTORE_LOCATION, truststorePath);
X509Util.enableCertFileReloading(myConf, keystoreWatcher,
truststoreWatcher, () -> {
});
assertNotNull(keystoreWatcher.get());
- assertThat(keystoreWatcher.get().getWatcherThreadName(),
Matchers.endsWith("-foo.jks"));
+ assertThat(keystoreWatcher.get().getWatcherThreadName(),
Matchers.endsWith("foo.jks"));
assertNotNull(truststoreWatcher.get());
- assertThat(truststoreWatcher.get().getWatcherThreadName(),
Matchers.endsWith("-bar.jks"));
+ assertThat(truststoreWatcher.get().getWatcherThreadName(),
Matchers.endsWith("bar.jks"));
keystoreWatcher.getAndSet(null).stop();
truststoreWatcher.getAndSet(null).stop();
}
@Test
- public void testCallbackWorksOnFileChanges() throws IOException,
InterruptedException {
+ public void testNoFalseNotifications() throws IOException,
InterruptedException {
FileChangeWatcher watcher = null;
try {
- final List<WatchEvent<?>> events = new ArrayList<>();
- watcher = new FileChangeWatcher(tempDir.toPath(), "test", event -> {
- LOG.info("Got an update: {} {}", event.kind(), event.context());
- // Filter out the extra ENTRY_CREATE events that are
- // sometimes seen at the start. Even though we create the watcher
- // after the file exists, sometimes we still get a create event.
- if (StandardWatchEventKinds.ENTRY_CREATE.equals(event.kind())) {
- return;
- }
- synchronized (events) {
- events.add(event);
- events.notifyAll();
+ final List<Path> notifiedPaths = new ArrayList<>();
+ watcher = new FileChangeWatcher(tempFile.toPath(), "test",
POLL_INTERVAL, path -> {
+ LOG.info("Got an update on path {}", path);
+ synchronized (notifiedPaths) {
+ notifiedPaths.add(path);
+ notifiedPaths.notifyAll();
}
});
watcher.start();
watcher.waitForState(FileChangeWatcher.State.RUNNING);
Thread.sleep(1000L); // TODO hack
Review Comment:
The purpose of the sleep here is to give time for the watcher thread to
notify the above callback of file modifications, if it chooses to. It's not
supposed to notify at this point, so removing the sleep would mean losing the
opportunity to catch a bug. Of course, there's always a way to refactor this
without the sleep, but not simply.
--
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]
