With WatcherService, when events are missed (which is to be expected), you will still need to list the files. It seems to me that WatcherService doesn't offer significant benefits in this case.
Regarding listing directory with a refresh flag, my concern is the potential for abuse. End-users might/could always refresh before listing, which could undermine the purpose of caching. Perhaps Jeremiah can provide more insight on this. IMO, caching is best handled internally. I have a few UX-related questions: - Is it valid or acceptable to return stale data? If so, end-users have to do some form of validation before consuming each snapshot to account for potential deletions. - Even if listsnapshot returns the most recent data, is it possible that some of the directories get deleted when end-users are accessing them? I think it is true. It, then, enforces end-users to do some validation first, similar to handling stale data. Just my 2 cents. - Yifan On Wed, Aug 7, 2024 at 6:03 AM Štefan Miklošovič <smikloso...@apache.org> wrote: > Yes, for example as reported here > > https://issues.apache.org/jira/browse/CASSANDRA-13338 > > People who are charting this in monitoring dashboards might also hit this. > > On Wed, Aug 7, 2024 at 2:59 PM J. D. Jordan <jeremiah.jor...@gmail.com> > wrote: > >> If you have a lot of snapshots and have for example a metric monitoring >> them and their sizes, if you don’t cache it, creating the metric can cause >> performance degradation. We added the cache because we saw this happen to >> databases more than once. >> >> > On Aug 7, 2024, at 7:54 AM, Josh McKenzie <jmcken...@apache.org> wrote: >> > >> > >> >> >> >> Snapshot metadata are currently stored in memory / they are cached so >> we do not need to go to disk every single time we want to list them, the >> more snapshots we have, the worse it is. >> > Are we enumerating our snapshots somewhere on the hot path, or is this >> performance concern misplaced? >> > >> >> On Wed, Aug 7, 2024, at 7:44 AM, Štefan Miklošovič wrote: >> >> Snapshot metadata are currently stored in memory / they are cached so >> we do not need to go to disk every single time we want to list them, the >> more snapshots we have, the worse it is. >> >> >> >> When a snapshot is _manually_ removed from disk, not from nodetool >> clearsnapshot, just by rm -rf on a respective snapshot directory, then such >> snapshot will be still visible in nodetool listsnapshots. Manual removal of >> a snapshot might be done e.g. by accident or by some "impatient" operator >> who just goes to disk and removes it there instead of using nodetool or >> respective JMX method. >> >> >> >> To improve UX here, what I came up with is that we might use Java's >> WatchService where each snapshot dir would be registered. WatchService is >> part of Java, it uses inotify subsystem which is what Linux kernel offers. >> The result of doing it is that once a snapshot dir is registered to be >> watched and when it is removed then we are notified about that via inotify >> / WatchService so we can react on it and remove the in-memory >> representation of that so it will not be visible in the output anymore. >> >> >> >> While this works, there are some questions / concerns >> >> >> >> 1) What do people think about inotify in general? I tested this on 10k >> snapshots and it seems to work just fine, nevertheless there is in general >> no strong guarantee that every single event will come through, there is >> also a family of kernel parameters around this where more tuning can be >> done etc. It is also questionable how this will behave on other systems >> from Linux (Mac etc). While JRE running on different platforms also >> implements this, I am not completely sure these implementations are >> quality-wise the same as for Linux etc. There is a history of >> not-so-quality implementations for other systems (events not coming through >> on Macs etc) and while I think we are safe on Linux, I am not sure we want >> to go with this elsewhere. >> >> >> >> 2) inotify brings more entropy into the codebase, it is another thing >> we need to take care of etc (however, it is all concentrated in one class >> and pretty much "isolated" from everything else) >> >> >> >> I made this feature optional and it is turned off by default so people >> need to explicitly opt-in into this so we are not forcing it on anybody. >> >> >> >> If we do not want to go with inotify, another option would be to have >> a background thread which would periodically check if a manifest exists on >> a disk, if it does not, then a snapshot does not either. While this works, >> what I do not like about this is that the primary reason we moved it to >> memory was to bypass IO as much as possible yet here we would introduce >> another check which would go to disk, and this would be done periodically, >> which beats the whole purpose. If an operator lists snapshots once a week >> and there is a background check running every 10 minutes (for example), >> then the cummulative number of IO operations migth be bigger than us just >> doing nothing at all. For this reason, if we do not want to go with >> inotify, I would also not implement any automatic background check. Instead >> of that, there would be SnapshotManagerMbean#refresh() method which would >> just forcibly reload all snapshots from scratch. I think that manual >> deletion of snapshots is not something a user would do regularly, snapshots >> are meant to be removed via nodetool or JMX. If manual removal ever happens >> then in order to make it synchronized again, the refreshing of these >> snapshots would be required. There might be an additional flag in nodetool >> listsnapshots, once specified, refreshing would be done, otherwise not. >> >> >> >> How does this all sound to people? >> >> >> >> Regards >> > >> >
