-
-
Notifications
You must be signed in to change notification settings - Fork 922
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
Polling fallback #9
Comments
FYI, the link (above) to the goconvey scanner is no longer valid (I've recently rewritten that package. My approach, however, is unchanged: https://github.com/smartystreets/goconvey/tree/master/web/server/watch |
👍 for polling. It is not ideal, but it is better than none and instead of everyone developing it on their own, it makes sense to have one that is made better by many people. In terms of the opt-in option, |
I've never even thought of extending it to webhook notifications like Dropbox and Google Drive provide. Interesting idea. Not sure if it belongs in this package, but having a common interface in top could be cool. The sql driver model is one I've been thinking about, particularly if there are multiple options for a given OS. |
Yeah, it really depends how you look at it, I wouldn't consider them as general webhooks as you can think of dropbox or s3 just another type of filesystem. Regardless of that, having a watch-driver interface and different driver is a better design IMHO. |
Indeed. It also fits with another desire I have, which is to make it easier to contribute to without being knowledgable of every single platform. Separate drivers for inotify, kqueue, etc. would be one way to achieve that. |
👍 polling would be great. I'm trying to migrate a python application from watchdog to fsnotify. Unfortunately I think this is the roadblock, as the filesystem that needs monitored is on NFS and polling is the only option. |
Yah, polling is the only option for NFS as far as I know. You can also check https://github.com/rjeczalik/notify but I don't think it has polling yet either. |
You might be interested in checking how docker support fsnotify and polling at the same time. We have a compatible interface and a fallback initialization for when fsnotify is not supported: https://github.com/docker/docker/blob/master/pkg/filenotify/filenotify.go maybe @cpuguy83 would be interested in moving the polling here so we can maintain only one package. |
That would be great. |
Please do submit a pull request. |
@nathany Any plans for the v2 with driver interfaces? I will be happy to help out. |
@omeid I don't have any plans for what a driver interface would look like yet (and to be honest, I haven't done much work on fsnotify lately). Personally, I'd prefer to see the current code base cleaned up before doing a big API change. The Windows internals are particularity crufty. Maybe we could start a new issue to figure out the details of transitioning to a driver model? |
@nathany Can you clarify what you'd like to see? |
@cpuguy83 Do you think you would be able to add polling without changing the API? Maybe just for operating systems that fsnotify doesn't currently support? There are other situations where polling would be desirable, but I'm not sure how to detect them, or if it should be done more manually (which is why a driver-style API is relevant to this discussion). |
@mdwhatcott @cpuguy83 Would you be willing to build a stand-alone If so, I'll create a repo called polling or poller or whatever you prefer. For me the key considerations are:
|
@nathany - Sounds like a fun project but I can't commit to it at this time. |
ok. Here is a polling watcher by @radovskyb https://github.com/radovskyb/watcher |
@radovskyb Hey Benjamin, Would you be interested in transferring watcher into the fsnotify organization and working on it here? Still as a stand-alone repository for the time being. The API already looks pretty close to fsnotify. Once some of the low-level bits are extracted from fsnotify (e.g. #173) I'd like to incorporate polling into fsnotify as a fallback, while still allowing people to use the poller directly if that's all they want. |
I need this package in order to port auditbeat software (https://github.com/elastic/beats) on AIX. |
Just wanted to add my two cents about polling mechanism. In Kubernetes, when a ConfigMap which is attached as a file on a pod changes, the file inside the pod changes as well but its last modified timestamp does not change. Size may not change depending on the change. So the GoConvey's polling approach will not work on this scenario. |
@bep Recently added a polling fallback in Hugo that may be worth taking a look at. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Started working on some implementation for this issue, initial thoughts can be seen here. It only implements the recursive version for now and has a few bugs related to watching new added items (and/or improvements). It uses multi pool workers to watch a batch of items (folders and files) in some interval - which comes from a random range. It also uses bloom filters to detect new items. |
Re: your email from yesterday:
It's just that no one wrote any code for it. Or when they did, the code wasn't really merge-able for one reason or the other. For example that existing PR only has recursion. The minimum requirements for it to get merged is:
Other than that I haven't looked too closely at your PR; there are just some high-level comments. Actually running the tests for the poll watcher is not so straight-forward, because everything is assumed to be GOOS-based. I did a bit of work to start moving away from that a few weeks ago by splitting the Watcher type from the backends, but there's still quite a bit of work to do especially for tests. Should probably make a "runAllTests(b backend)" or something, but this requires some refactoring and such. The supports...() for running optional tests should probably also be moved to the backend. All of this is supposed to be temporary until it's implemented everywhere. All of that should be a separate PR. I don't mind working on that, but I don't know when. I'm mostly focused on getting the optional features in all backends. Another general issue is what the API should look like; I spent a bit of time on it a few weeks ago; but I'm not really sure about this yet. At the very least I would like to have options to 1) set the poll interval, 2) check file contents based on hash, and 3) maximum number of parallel goroutines to scan with. Those last two don't need to be in the first version, but do need to think about the API. |
Hey, the case that i'm working with aims for very very large directories and also lots of directories/subdirectories. I did not perform any considerable test so far, but for development i'm using ~6k folders and subfolders with a total of 50k files (~10 level deep), which is performing well. One of the things that i'm adding is the option to select the interval and the change based on file hash. Actually, my only idea for supporting the rename so far is using the file hash - detecting a new file that has same hash as a deleted file (let me know if you have some thoughts). The bloomfilter I believe is a perfect fit here for tracking new files, I don't see any reason to avoid it... it would only be using more memory to achieve the same end result. Related to ants pool, probably is something that can be replaced by an internal pool implementation, but in a scenario with lots of tasks it is better to work with it (at least I think), considering that we can benefit from multi pools, task queues, cached workers as well as cleanup. My idea on the use of ants is achieve better performance and memory usage. For my solution I would say that both this dependencies are essential, ants and bloomfilter (I have to remove the one for maxproc). I also want to add injection of a Logger, I'm missing it in the current usage. Another consideration for it to be considered a valid implementation here, would be to change the way that the directories are scanned, using it with os.Open means that the physical directory gets locked, which I'm able to deal well in my company scenario because of internal lock system, but here would be problematic. But, overall I think this could bring good insights and valuable discussion (for me is important because I need to read the directory as a buffered input in batches). |
This is a generic project used by lots of people in all sorts of cases, and your use case is just one, and the polling backend is just one backend. Adding a bunch of dependencies here means adding a bunch of dependencies to 250,000 projects. If we want to optimize things with a bloom filter then we can always do that later. And that optimisation also applies to kqueue, and intotify, and everything else. And maybe a tree is an even better optimisation. But realistically, I would expect it doesn't really matter too much in the context of file I/O, which is much slower. This is like optimising the string concatenation of your SQL queries when those queries take a second to run.
This should be a different PR/issue. |
I mean... I know that, that's why it is in my repository only right? Was only meaning to help, but well... thanks for the project, good luck :) |
Well you did ask if a patch could be accepted. Anyway, I took a closer look at this today; all operations below done on a collection of 50k items, collected from my system's /usr (to give reasonable realistic strings):
Performance wise the end result is that it's more or less even except deletes are much slower. Unacceptably slow in fact (more on that later). It might save a bit of memory, but I doubt anyone will notice, and I say "might" because you will need to keep a separate list of files regardless for WatchList() and perhaps other things, which you probably won't need to do for a map (which you can re-use). Another downside is that bloom filters have the chance of false positives. If I test all 121,693 paths entries in my /usr against that list of 50k a map will give exactly 50k matches, as expected, but the bloom filter gives me 50,469 matches. This means that in some cases it may report events for paths that didn't actually get any events. These things may be somewhat rare but do happen (e.g. browser safe search uses a bloom filter for URL lookups, which has had false positives in the past due to this). That's with fp=0.01 (as per your patch). Tweaking this value I get fp=0.001 50042, fp=0.0001 50008, fp=0.00001 50k. Tweaking this value to 0.00001 makes appends (and by extension, the already slow delete) about twice as slow, to 12ms, and it will use 146K of memory. Deleting 100 files would mean spending 1.2 seconds just on creating new bloom filters. Deleting 1,000 files would be 12 seconds. Yikes! Unless I spectacularly missed something, the conclusion seems obvious given the above: bloom filters are absolutely not suitable for the general case, and they are probably not suitable for your use case either. It will regress performance significantly in the hopes that it may perhaps save some memory and offer no advantages at beyond that. |
Whether or not fsnotify implements polling support itself, some thought should be given into how polling could work as an alternative to native OS events.
GoConvey uses polling exclusively to avoid the "too many files" error #8.
https://github.com/smartystreets/goconvey/blob/master/web/server/watcher/scanner.go
@pifantastic reports that node's gaze (https://github.com/shama/gaze#errors) library detects
EMFILE
errors and falls back to polling.Polling could be opt-in for network file systems (NFS), Vagrant or Plan 9 -- where OS events don't work or are unavailable.
The text was updated successfully, but these errors were encountered: