-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ISPN-15916 Certificate reloading #12359
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've modified it to use plain last modified time
e711b60
to
97b66e5
Compare
public void run() { | ||
while (running) { | ||
try { | ||
Thread.sleep(SLEEP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also used by the client.
/** | ||
* @since 15.0 | ||
*/ | ||
public class FileWatcher implements Runnable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial implementation did use that, but read #12359 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. Sorry, I only looked at the code not the prior comments :D
commons/all/src/main/java/org/infinispan/commons/io/FileWatcher.java
Outdated
Show resolved
Hide resolved
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 |
d490790
to
a4fdc4b
Compare
a3b168a
to
6670ed2
Compare
6670ed2
to
b61c0d3
Compare
b61c0d3
to
ee6c3d4
Compare
Test failures are unrelated. |
Thanks @tristantarrant |
https://issues.redhat.com/browse/ISPN-15916