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

Improve comment in db_impl.h #5338

Closed
wants to merge 1 commit into from
Closed

Conversation

siying
Copy link
Contributor

@siying siying commented May 23, 2019

Summary:
Add some comments in db_impl.h. Also reordered function order a little bit so that I can add a comment to flag the area of functions implementing DB interface.

Test Plan: make check with both of LITE and normal build

@siying
Copy link
Contributor Author

siying commented May 24, 2019

ping

db/db_impl.h Outdated
@@ -75,16 +75,31 @@ struct JobContext;
struct ExternalSstFileInfo;
struct MemTableInfo;

// While DB is the public interface of RocksDB, and DBImpl is the actual
// class implementing it. It's the entrance of the core RocksdB engine.
// All other DB implementations, e.g. TransactionDB, BlobDB, etc, wraps a
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap?

db/db_impl.h Outdated
// All other DB implementations, e.g. TransactionDB, BlobDB, etc, wraps a
// DBImpl internally.
// Other than functions implementing the DB interface, some public
// functions are there for other internal components to call. It includes
Copy link
Contributor

Choose a reason for hiding this comment

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

"It" refers to "some public functions"? maybe should be "They include"?

db/db_impl.h Outdated
// DBImpl internally.
// Other than functions implementing the DB interface, some public
// functions are there for other internal components to call. It includes
// components of higher layer. For example, TransactionDB directly
Copy link
Contributor

Choose a reason for hiding this comment

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

"higher layers" or "the high layer"?

db/db_impl.h Outdated
// components of higher layer. For example, TransactionDB directly
// calls DBImpl::WriteImpl() and BlobDB directly calls
// DBImpl::GetImpl(). Other functions are public for sub-componets
// to interace. For example, ColumnFamilyHandleImpl calls into
Copy link
Contributor

Choose a reason for hiding this comment

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

"calls into" -> "calls"?

// mode and LastAllocatedSequence otherwise. This is useful when visiblility
// depends also on data written to the WAL but not to the memtable.
SequenceNumber TEST_GetLastVisibleSequence() const;

Copy link
Contributor

Choose a reason for hiding this comment

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

redundant new line?

db/db_impl.h Outdated
// load list of snapshots to `snap_vector` that is no newer than `max_seq`
// in ascending order.
// `oldest_write_conflict_snapshot` is filled with the oldest snapshot
// which satifies SnapshotImpl.is_write_conflict_boundary_ = true.
Copy link
Contributor

Choose a reason for hiding this comment

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

satisfies

db/db_impl.h Outdated
@@ -748,6 +701,8 @@ class DBImpl : public DB {

InstrumentedMutex* mutex() const { return &mutex_; }

// Initial a brand new DB. The DB directory is expected to be empty before
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialize?

Copy link
Contributor

@miasantreble miasantreble left a comment

Choose a reason for hiding this comment

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

LGTM, left some comments about wording

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.

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary:
Add some comments in db_impl.h. Also reordered function order a little bit so that I can add a comment to flag the area of functions implementing DB interface.

Test Plan: make check with both of LITE and normal build

Some wording improvements.

Address comments.
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.

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@siying merged this pull request in 6267ed2.

vagogte pushed a commit to vagogte/rocksdb that referenced this pull request Jun 18, 2019
Summary:
Add some comments in db_impl.h. Also reordered function order a little bit so that I can add a comment to flag the area of functions implementing DB interface.
Pull Request resolved: facebook#5338

Differential Revision: D15498284

Pulled By: siying

fbshipit-source-id: 3d7c59c8303577fe44d13c74ae84c7ce05164f77
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