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

Introduce a CountedFileSystem for counting file operations #9283

Closed
wants to merge 17 commits into from

Conversation

mrambacher
Copy link
Contributor

Added a CountedFileSystem that tracks a number of file operations (opens, closes, deletes, renames, flushes, syncs, fsyncs, reads, writes). This class was based on the ReportFileOpEnv from db_bench.

This is a stepping stone PR to be able to change the SpecialEnv into a SpecialFileSystem, where several of the file varieties wish to do operation counting.

Added a CountedFileSystem that tracks a number of file operations (opens, closes, deletes, renames, flushes, syncs, fsyncs, reads, writes).    This class was based on the ReportFileOpEnv from db_bench.

This is a stepping stone PR to be able to change the SpecialEnv into a SpecialFileSystem, where several of the file varieties wish to do operation counting.
@facebook-github-bot
Copy link
Contributor

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

@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 imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

Generally looks OK

}
};

struct FileCounters {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of this name, as it sounds like it counts things associated with each file rather than across all files. I like FileOpCounters better than FileCounters though I don't have great ideas for the other one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to FileOpCounters

std::atomic<int> opens;
std::atomic<int> deletes;
std::atomic<int> renames;
std::atomic<int> flushes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably OK if this is only ever used in unit tests, but I would expect this to overflow if ever used in production. Likely also could in benchmarking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were taken from ReportFileOpCounters in db_bench_tool.cc. If they should be uint64_t, I have no problems changing them.

Copy link
Contributor

Choose a reason for hiding this comment

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

uint64_t is my preference here.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

- Added NewDirectory to allow counting of Fsync from Directories
- Added PrintCounters to dump the counters to a string.
- Added ResetCounters to clear the counters
- Remove skip_fsync
@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@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

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

👍

std::atomic<int> opens;
std::atomic<int> deletes;
std::atomic<int> renames;
std::atomic<int> flushes;
Copy link
Contributor

Choose a reason for hiding this comment

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

uint64_t is my preference here.

@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 imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

satyajanga added a commit to satyajanga/rocksdb that referenced this pull request Feb 3, 2022
Introduce a CountedFileSystem for counting file operations (facebook#9283)
@mrambacher mrambacher deleted the CountedFileSystem branch May 16, 2022 16:32
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