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 file inspector and add FileSystemInspectedEnv #216

Merged
merged 15 commits into from
Dec 21, 2020

Conversation

tabokie
Copy link
Member

@tabokie tabokie commented Dec 3, 2020

Introduce FileSystemInspector abstract interface and FileSystemInspectedEnv. With FileSystemInspectedEnv, each file operation must first consult FileSystemInspector before reaching actual IO logic.
Also, add IOType support to SequentialFile. Before compaction or flush, the specific type can be passed to inspector through newly added methods. Notice it might be possible to merge existing TableReaderCaller with IOType, maybe as a followup task.

Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
@@ -364,6 +364,8 @@ class Env {
// Priority for requesting bytes in rate limiter scheduler
enum IOPriority { IO_LOW = 0, IO_HIGH = 1, IO_TOTAL = 2 };

enum IOType { IO_UNCATEGORIZED = 0, IO_FLUSH = 1, IO_COMPACTION = 2 };
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to avoid adding IO type to rocksdb code. I think the rust FileSystemInpector can obtain the thread-local IO type by itself. To tell flush and compaction apart, we can set the thread-local IO type in the OnFlushBegin/OnCompactionBegin callbacks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great, save lots of trouble.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yiwu-arbug But I think OnCompactionBegin doesn't respect subcompaction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sad. how about adding an sub-compaction begin event?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also OnFlushComplete is tricky, it can run on another thread. But maybe we just make use of both of the Begin event.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's doable, I'll leave it for followup PRs.

Copy link
Collaborator

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

sorry for delay.

env/env_inspected.cc Outdated Show resolved Hide resolved
env/env_inspected.cc Show resolved Hide resolved
env/env_inspected.cc Outdated Show resolved Hide resolved
env/env_inspected.cc Show resolved Hide resolved
env/env_inspected.cc Outdated Show resolved Hide resolved
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
Copy link
Member Author

tabokie commented Dec 18, 2020

Comments addressed. @yiwu-arbug @Connor1996

Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Copy link
Collaborator

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@tabokie tabokie deleted the inspect-fs branch December 23, 2020 03:17
@tabokie tabokie mentioned this pull request May 9, 2022
39 tasks
tabokie added a commit to tabokie/rocksdb that referenced this pull request May 12, 2022
Introduce `FileSystemInspector` interface and `FileSystemInspectedEnv`. With
`FileSystemInspectedEnv`, each file operation must first consult
`FileSystemInspector` before reaching actual IO logic.

Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit that referenced this pull request May 12, 2022
Introduce `FileSystemInspector` interface and `FileSystemInspectedEnv`. With
`FileSystemInspectedEnv`, each file operation must first consult
`FileSystemInspector` before reaching actual IO logic.

Signed-off-by: tabokie <xy.tao@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants