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.

Sign up for GitHub

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

Jump to bottom

[IOTracing PR3] Store FSRandomAccessPtr object in RandomAccessFileReader #7192

Closed
wants to merge 2 commits into from
Closed

[IOTracing PR3] Store FSRandomAccessPtr object in RandomAccessFileReader #7192

wants to merge 2 commits into from

Conversation

akankshamahajan15
Copy link
Contributor

Summary: Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
object in RandomAccessFileReader.
This new object wraps FSRandomAccessFile pointer.

Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
FSRandomAccessFileTracingWrapper pointer that includes all necessary
information in IORecord and calls underlying FileSystem and invokes
IOTracer to dump that record in a binary file. If tracing is disabled
then, underlying FileSystem pointer is returned directly.
FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
tracing is disabled.

Test Plan: make check -j64

Reviewers:

Subscribers:

Tasks:

Tags:

@akankshamahajan15 akankshamahajan15 force-pushed the store_FSRandomAccessFilePtr branch 2 times, most recently from ac697cd to 8b72252 Compare August 13, 2020 22:18
@akankshamahajan15 akankshamahajan15 marked this pull request as ready for review August 13, 2020 22:22
zhichao-cao
zhichao-cao reviewed Aug 17, 2020
View reviewed changes
db/version_set.cc Outdated Show resolved Hide resolved
zhichao-cao
zhichao-cao reviewed Aug 17, 2020
View reviewed changes
file/file_util.h Outdated Show resolved Hide resolved
anand1976
anand1976 reviewed Aug 18, 2020
View reviewed changes
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

LGTM

env/file_system_tracer.h Outdated Show resolved Hide resolved
env/file_system_tracer.h Outdated Show resolved Hide resolved
@akankshamahajan15
[IOTracing PR3] Store FSRandomAccessPtr object in RandomAccessFileReader
619185b 
Summary: Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
object in RandomAccessFileReader.
This new object wraps FSRandomAccessFile pointer.

Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
FSRandomAccessFileTracingWrapper pointer that includes all neccessary
information in IORecord and calls underlying FileSystem and invokes
IOTracer to dump that record in a binary file. If tracing is disabled
then, underlying FileSystem pointer is returned directly.
FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
tracing is disabled.

Test Plan: make check -j64

Reviewers:

Subscribers:

Tasks:

Tags:
anand1976
anand1976 reviewed Aug 26, 2020
View reviewed changes
file/random_access_file_reader.h Outdated
RandomAccessFileReader(RandomAccessFileReader&& o) ROCKSDB_NOEXCEPT {
*this = std::move(o);
RandomAccessFileReader(RandomAccessFileReader&& o) ROCKSDB_NOEXCEPT
: file_(std::move(o.file_)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for std::move of individual member variables instead of std::move(o)? Also, what about file_name_ and listeners_?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In std::move(o), it expects FSRandomAccessFilePtr to have default constructor, which in turns expect FSRandomAccessFileTracingWrapper(since its a object) to have default Constructor but FSRandomAccessFileTracingWrapper is inherited from FSRandomAccessFileWrapper and it expects pointer to file system (No default constructor). So I added it to do member by member move. (similar to move operator). But i wanted to remove these move functions as operator= also doesn't have file_name_ and listeners_ .

env/file_system_tracer.h

FSSequentialFile* operator->() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return fs_tracer_.get();
return const_cast<FSSequentialFileTracingWrapper*>(&fs_tracer_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is const_cast necessary, since FSSequentialFileTracingWrapper is a descendant of FSSequentialFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is giving error without const cast because function is defined const. So I had to add the type cast

@akankshamahajan15
Addressed comments
758b537 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@akankshamahajan15
Copy link
Contributor Author

Updated RandomAccessFileRandom to use implicit move constructor and move operator.

anand1976
anand1976 approved these changes Aug 26, 2020
View reviewed changes
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

facebook-github-bot
facebook-github-bot reviewed Aug 26, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

zhichao-cao
zhichao-cao approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@zhichao-cao zhichao-cao left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing the comments!

@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 merged this pull request in 8e0df90.

codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
@akankshamahajan15 @codingrhythm
Store FSRandomAccessPtr object in RandomAccessFileReader (facebook#7192)
7791fc4 
Summary:
Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
    object in RandomAccessFileReader.
    This new object wraps FSRandomAccessFile pointer.

    Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
    FSRandomAccessFileTracingWrapper pointer that includes all necessary
    information in IORecord and calls underlying FileSystem and invokes
    IOTracer to dump that record in a binary file. If tracing is disabled
    then, underlying FileSystem pointer is returned directly.
    FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
    tracing is disabled.

    Test Plan: make check -j64

Pull Request resolved: facebook#7192

Reviewed By: anand1976

Differential Revision: D23356867

Pulled By: akankshamahajan15

fbshipit-source-id: 48f31168166a17a7444b40be44a9a9d4a5c7182c
levichen94 pushed a commit to bytedance/terarkdb that referenced this pull request Jun 18, 2021
@akankshamahajan15
Store FSRandomAccessPtr object in RandomAccessFileReader (#7192)
c41f2f5 
Summary:
Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
    object in RandomAccessFileReader.
    This new object wraps FSRandomAccessFile pointer.

    Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
    FSRandomAccessFileTracingWrapper pointer that includes all necessary
    information in IORecord and calls underlying FileSystem and invokes
    IOTracer to dump that record in a binary file. If tracing is disabled
    then, underlying FileSystem pointer is returned directly.
    FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
    tracing is disabled.

    Test Plan: make check -j64

Pull Request resolved: facebook/rocksdb#7192

Reviewed By: anand1976

Differential Revision: D23356867

Pulled By: akankshamahajan15

fbshipit-source-id: 48f31168166a17a7444b40be44a9a9d4a5c7182c
Signed-off-by: Changlong Chen <levisonchen@live.cn>
levichen94 pushed a commit to bytedance/terarkdb that referenced this pull request Jun 18, 2021
@akankshamahajan15
Store FSRandomAccessPtr object in RandomAccessFileReader (#7192)
956264b 
Summary:
Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
    object in RandomAccessFileReader.
    This new object wraps FSRandomAccessFile pointer.

    Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
    FSRandomAccessFileTracingWrapper pointer that includes all necessary
    information in IORecord and calls underlying FileSystem and invokes
    IOTracer to dump that record in a binary file. If tracing is disabled
    then, underlying FileSystem pointer is returned directly.
    FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
    tracing is disabled.

    Test Plan: make check -j64

Pull Request resolved: facebook/rocksdb#7192

Reviewed By: anand1976

Differential Revision: D23356867

Pulled By: akankshamahajan15

fbshipit-source-id: 48f31168166a17a7444b40be44a9a9d4a5c7182c
Signed-off-by: Changlong Chen <levisonchen@live.cn>
mm304321141 pushed a commit to bytedance/terarkdb that referenced this pull request Jun 23, 2021
@akankshamahajan15 @mm304321141
Store FSRandomAccessPtr object in RandomAccessFileReader (#7192)
ee4b87a 
Summary:
Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
    object in RandomAccessFileReader.
    This new object wraps FSRandomAccessFile pointer.

    Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
    FSRandomAccessFileTracingWrapper pointer that includes all necessary
    information in IORecord and calls underlying FileSystem and invokes
    IOTracer to dump that record in a binary file. If tracing is disabled
    then, underlying FileSystem pointer is returned directly.
    FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
    tracing is disabled.

    Test Plan: make check -j64

Pull Request resolved: facebook/rocksdb#7192

Reviewed By: anand1976

Differential Revision: D23356867

Pulled By: akankshamahajan15

fbshipit-source-id: 48f31168166a17a7444b40be44a9a9d4a5c7182c
Signed-off-by: Changlong Chen <levisonchen@live.cn>
mm304321141 pushed a commit to bytedance/terarkdb that referenced this pull request Jun 23, 2021
@akankshamahajan15 @mm304321141
Store FSRandomAccessPtr object in RandomAccessFileReader (#7192)
faa3d7d 
Summary:
Replace FSRandomAccessFile pointer with FSRandomAccessFilePtr
    object in RandomAccessFileReader.
    This new object wraps FSRandomAccessFile pointer.

    Objective: If tracing is enabled, FSRandomAccessFile Ptr returns
    FSRandomAccessFileTracingWrapper pointer that includes all necessary
    information in IORecord and calls underlying FileSystem and invokes
    IOTracer to dump that record in a binary file. If tracing is disabled
    then, underlying FileSystem pointer is returned directly.
    FSRandomAccessFilePtr wrapper class is added to bypass the FSRandomAccessFileWrapper when
    tracing is disabled.

    Test Plan: make check -j64

Pull Request resolved: facebook/rocksdb#7192

Reviewed By: anand1976

Differential Revision: D23356867

Pulled By: akankshamahajan15

fbshipit-source-id: 48f31168166a17a7444b40be44a9a9d4a5c7182c
Signed-off-by: Changlong Chen <levisonchen@live.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zhichao-cao zhichao-cao zhichao-cao approved these changes

@anand1976 anand1976 anand1976 approved these changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@akankshamahajan15 @facebook-github-bot @zhichao-cao @anand1976