Create snapshot when listing contents of a path. #1805
Conversation
# Conflicts: # test/src/common/harnessStorage.c
|
Continuing my review and should finish up by lunch tomorrow (PST). I'm not sure if it better to have a series of smaller PRs or a single big one like this one. Either way it amounts to the same work for reviewing, but I find myself longing for bite-sized pieces. Will continue in the AM. |
In general we try to break patches up at much as possible, but this one was not as amenable to being split. |
There was a problem hiding this comment.
I think we can further refine the way iterators are used, but this code is pretty good. My thinking is the storage driver API should return a single level iterator (which would internally include the storage list in some cases). We would build the universal recursive scan on top of the driver's single level scan. This is something to consider for the future to make the various drivers a bit more similar to each other.
Looks good. I'm in favor of fixing that missing "else", not because it is doing any harm, but because it looks so much like a bug I'm uncomfortable leaving it in place.
Agreed. This would work for the object stores where we currently still have callbacks as well.
Done in b54d25c. I was just holding it until you finished your review. |
Previously a callback was used to list path contents and if no sort was specified then a snapshot was not required. When deleting files from the path some filesystems could omit files that still existed, which meant the path could not be removed.
Filter . out of lists in the Posix driver since this special entry was only used by test code (and filtered everywhere in the core code).
Also remove callbacks from the storage interface and replace with an iterator that should be easier to use and guarantees efficient use of the snapshots.