ISPN-15916 Certificate reloading #12359
Conversation
| private static FileWatcher INSTANCE; | ||
|
|
||
| private final Thread thread; | ||
| private final WatchService watchService; |
There was a problem hiding this comment.
This won't work as expected in K8s:
https://blog.arkey.fr/2019/09/13/watchservice-and-bind-mount/
There was a problem hiding this comment.
I've modified it to use plain last modified time
tristantarrant
force-pushed
the
ISPN-15916/ssl_reload
branch
from
e711b60
to
97b66e5
Compare
| public void run() { | ||
| while (running) { | ||
| try { | ||
| Thread.sleep(SLEEP); |
There was a problem hiding this comment.
Shouldn't we be making use of a ScheduledExecutorService and in the case of the server should we be creating this via NamedExecutorsFactory?
There was a problem hiding this comment.
This is also used by the client.
| /** | ||
| * @since 15.0 | ||
| */ | ||
| public class FileWatcher implements Runnable { |
There was a problem hiding this comment.
Do we want to use a WatchService instead? https://docs.oracle.com/javase/tutorial/essential/io/notification.html It should theoretically be more lightweight (don't have to check all registered files) and more responsive when file system supports (no sleeps required).
There was a problem hiding this comment.
The initial implementation did use that, but read #12359 (comment)
There was a problem hiding this comment.
Ah, yes. Sorry, I only looked at the code not the prior comments :D
| private volatile boolean running = true; | ||
|
|
||
| public static synchronized void watch(Path path, Consumer<Path> callback) throws IOException { | ||
| if (INSTANCE == null) { |
There was a problem hiding this comment.
If we keep this class, this can be done with initialization on demand idiom or enum singleton instead, removing the need for synchronization (especially since you are using a CHM).
wburns
changed the title
tristantarrant
force-pushed
the
ISPN-15916/ssl_reload
branch
from
97b66e5
to
9bbe0db
Compare
|
@tristantarrant This is breaking CI, there's also @wburns comment that needs addressing. |
The first few runs of this PR in CI were broken because the |
tristantarrant
force-pushed
the
ISPN-15916/ssl_reload
branch
from
9bbe0db
to
946c38a
Compare
https://issues.redhat.com/browse/ISPN-15916