-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Conversation
@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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done.
include/rocksdb/db.h
Outdated
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@ajkr has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done.
include/rocksdb/db.h
Outdated
// 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 |
There was a problem hiding this comment.
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.
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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
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