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

Fix a few documentation errors including in public APIs #9789

Closed
wants to merge 2 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Apr 1, 2022

The internal WriteBatch doc wrongly indicated which optypes are followed by varstring. Updated some optypes according to the following code:

rocksdb/db/write_batch.cc

Lines 418 to 429 in 76383be

case kTypeBeginPrepareXID:
// This indicates that the prepared batch is also persisted in the db.
// This is used in WritePreparedTxn
case kTypeBeginPersistedPrepareXID:
// This is used in WriteUnpreparedTxn
case kTypeBeginUnprepareXID:
break;
case kTypeEndPrepareXID:
if (!GetLengthPrefixedSlice(input, xid)) {
return Status::Corruption("bad EndPrepare XID");
}
break;

The Iterator::Refresh() + DeleteRange() bug was fixed in #9258; removed the warnings.

GetMergeOperands() does populate *number_of_operands including upon successful return:

rocksdb/db/db_impl/db_impl.cc

Lines 1917 to 1919 in 76383be

// Return all merge operands for get_impl_options.key
*get_impl_options.number_of_operands =
static_cast<int>(merge_context.GetNumOperands());

@facebook-github-bot
Copy link
Contributor

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

// kTypeBeginPrepareXID varstring
// kTypeEndPrepareXID
// kTypeBeginPrepareXID
// kTypeEndPrepareXID varstring
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an entry for kTypeCommitXIDAndTimestamp varstring varstring? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

// returned are in the order of insertion.
// merge_operands- Points to an array of at-least
// Assigns all the merge operands corresponding to the key to the
// `merge_operands` array. Upon success, the number of merge operands set is
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "number of merge operands set" a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrote to try to make it clearer.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor Author

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

// kTypeBeginPrepareXID varstring
// kTypeEndPrepareXID
// kTypeBeginPrepareXID
// kTypeEndPrepareXID varstring
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

// returned are in the order of insertion.
// merge_operands- Points to an array of at-least
// Assigns all the merge operands corresponding to the key to the
// `merge_operands` array. Upon success, the number of merge operands set is
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrote to try to make it clearer.

@facebook-github-bot
Copy link
Contributor

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

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