Skip to content
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

Create InjectionFileSystem/NoSyncFileSystem classes #9545

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mrambacher
Copy link
Contributor

The InjectionFileSystem is a base/abstract class that captures the IO methods associated with the FSxxxFile classes and allows the FileSystem to do something with them. For example, the CountedFileSystem can count the number of operations and bytes and the NoSyncFileSystem can skip the various Sync operations. The FaultInjectionFileSystem can insert the appropriate failures into the IO path.

The InjectionFileSystem is meant to make it easier to intercept these IO operations in a more standard and coherent way. Previously, every type of InjectionFileSystem would need to write their own FSxxxFile extension classes for each type/operation that is extended. This methodology leads to cases where some of the operations were missed in some implementations.

Note that the FaultInjectionFileSystem is not fully "Injection" ready, as it still has its own TestWritableFile class. This implementation will be cleaned up in a later PR. Additionally a later PR will make the SpecialEnv into a SpecialFileSystem taking advantage of these capabilities.

The NoSyncFileSystem is used for some tests to speed them up by skipping the Sync operations. This slowdown is especially noticeable on the Mac with the FULL_SYNC.

InjectionFileSystem is a base FileSystem that allows extensions to intercept calls to the File APIs (like Read, Append, Sync, etc).  This class allows those methods to be intercepted without writing new classes for all of the File (Sequential, Random, etc) classes.

NoSyncFileSystem is an implementation of an InjectionFileSystem that intercepts and optionally skips the various sync (Sync, FSync, RangeSync, Directory FSync) calls.
@facebook-github-bot
Copy link
Contributor

@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding NoSyncFileSystem, my opinion remains the same as #9356 (comment).
The ramdisk approach workaround for the slow F_FULLFSYNC on OSX should suffice. One limitation is that, after running the tests make check several times, the ramdisk may get full because some of the unit tests do not cleanup after completion, and you need to manually clean it up, which imho, is not a problem. I remember you also submitted #9539 to cleanup the unit tests directories, and I think this is the right direction if you think periodic manual cleaning up is intolerable.

@mrambacher
Copy link
Contributor Author

The ramdisk does not work so well on the mac. First, there is the problem that the tests do not clean up after themselves. The other problem is to run in parallel mode, even from a clean disk, will sometimes fail as some tests use a lot of space, exhausting the ramdisk.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

@mrambacher
Copy link
Contributor Author

The ramdisk does not work so well on the mac. First, there is the problem that the tests do not clean up after themselves. The other problem is to run in parallel mode, even from a clean disk, will sometimes fail as some tests use a lot of space, exhausting the ramdisk.

I might also add that the code for the NoSyncFileSystem is effectively copied from the SpecialEnv. A later PR can create a SpecialFileSystem that extends this new class. This will allow the classes who wish to use Special and those who do not to start from the same/similar Env/FileSystem classes.

@riversand963
Copy link
Contributor

First, there is the problem that the tests do not clean up after themselves.

Then this is the number 1 thing which I think we should fix.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants