Introducing a file group watcher #134564
Introducing a file group watcher #134564
Conversation
…built atop FileWatcher that invokes a callback once per group of file changes.
ankit--sethi
added
>non-issue
:Security/Security
elasticsearchmachine
added
serverless-linked
|
Pinging @elastic/es-security (Team:Security) |
| @Override | ||
| protected void doCheckAndNotify() throws IOException { | ||
| if (coalescingListener.filesChanged()) { | ||
| // fallback in case state wasn't cleared correctly in previous iteration |
There was a problem hiding this comment.
This sounds concerning. How can this happen?
There was a problem hiding this comment.
No, it shouldn't ever happen due to the reset happening in the finally block. I was thinking of very rare situations where the JDK spec says finally will not execute. I had initially found this reference:
Likewise, if the thread executing the try or catch code is interrupted or killed, the finally block may not execute even though the application as a whole continues
https://stackoverflow.com/a/2417986
After digging a little deeper to write this reply, it turns out this is a JDK documentation mistake that they have now updated. Apparently, the section I quoted above is not actually true!
https://bugs.openjdk.org/browse/JDK-8276156?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel
With things where they are now, I'm fine with removing this if condition if it scares more than helps!
There was a problem hiding this comment.
I've removed this if-condition
|
|
||
| static class CoalescingFileChangesListener implements FileChangesListener { | ||
|
|
||
| private final ThreadLocal<List<Path>> filesChanged = ThreadLocal.withInitial(() -> Collections.synchronizedList(new ArrayList<>())); |
There was a problem hiding this comment.
I'm failing to understand the need for ThreadLocal and synchronized list. Could you explain?
There was a problem hiding this comment.
Hmm, I think synchronized list was the result of an earlier solution that got superseded by ThreadLocal but I forgot to remove it. I'll explain the general scenario either way though:
- File change detected at the end of a particular time interval triggers this listener to action, the listener is executing in thread A.
- If the listener execution is time-consuming it is possible that thread A is still executing by the time a second monitoring interval completes. And if there is yet another change to the files, then the resource watcher will assign a second thread B to execute the same same listener. (I'm making some assumptions about how the resource watcher schedules tasks here)
- now thread A and B are modifying the same list concurrently.
This edge case requires CoalescingFileChangesListener to be doing something very time-consuming, or the watcher frequency is very very small. It's very unlikely but wrapping a list in the ThreadLocal is a low cost way to have some thread isolation in case this ever happens.
There was a problem hiding this comment.
If the listener execution is time-consuming it is possible that thread A is still executing by the time a second monitoring interval completes. And if there is yet another change to the files, then the resource watcher will assign a second thread B to execute the same same listener. (I'm making some assumptions about how the resource watcher schedules tasks here)
I think we should verify these assumptions with @elastic/es-core-infra (assuming you are the owners of resource watcher). My understanding is that file watcher does not schedule new thread until it finishes with processing current. Meaning, it seems to be single threaded.
There was a problem hiding this comment.
Sounds right, I tracked it down to these lines here and the flow is such that only after a task completes is the next runnable scheduled.
I feel though that depending on this information adds risk because it is tightly coupling the design to the internals of how a separate component works. With the ThreadLocal in place we don't need to depend on the scheduler guaranteeing thread safety, or things breaking if the design is ever modified.
There was a problem hiding this comment.
I've removed the synchronizedList usage, it's not required.
slobodanadamovic
added
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
…n-settings-change' into feature/recreate-http-client-when-settings-change
…ture/recreate-http-client-when-settings-change
|
I'm trying to understand the usecase. Can you describe a concrete use case that requires grouping in this way? |
We need to be able to watch multiple SSL configuration files and be notified if any of the watched files changes, instead of being notified for every file we are watching. We use notification as a trigger to build new Some background: Currently, We may not necessarily have to take the approach as proposed in this PR. And we could certainly take other approaches or implement security-specific workarounds, but it would be preferred to be able to tell watcher here are files to monitor, notify me once with grouped information about all changed files. |
We are working towards watching a directory being ok. The code in question would think the secure files do not exist, but would not receive any errors. I think this would be a more robust long term approach. The proposal here seems like a workaround that will maintenance/complexity in the long term. But aside from the above, I wonder if SSL service really needs to be in xpack core. It seems the only reason it is defined/exposed there is for xpack plugins to be able to create ssl capable clients. I wonder if this could be hidden away so that security implements this "create an ssl capable client", and then it can truly own these secure files. cc @mosche |
At the moment it does because monitoring, watcher, ML, etc all use it. We're working on reducing the need for other plugins to directly depend on the SSL service. The end point might get us to where it doesn't need to be in x-pack core anymore, but we're a number of steps away from that. |
|
In light of the discussion above, I'm closing this PR as we will eventually have a better solution native to File Watcher |
5 participants
Introducing the notion of a FileGroupWatcher, an orchestrating watcher built atop
FileWatcherthat invokes a callback only once per group of file changes.The motivation is to better handle cases where multiple files may have changed in a particular monitoring interval, yet the action we need to take should execute only once per set of changes. Triggering callbacks over a collection of File Watchers results in the same action being executed more number of times than is necessary.