-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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][docs] Improve docs for persistence policies parameters E, Qw and Qa. #18003
[improve][docs] Improve docs for persistence policies parameters E, Qw and Qa. #18003
Conversation
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.
Lgtm
Very useful
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.
These are good documentation improvements. I have previously read through the source code to verify this understanding.
I have two comments. The first is about the double negative in the documentation. The second is that sticky reads are also a bookkeeper client feature that likely could be documented on the bookkeeper website.
For example, here is the BK Javadoc for the config: https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java#L1181-L1198
It's completely off topic for this PR, but I think we could make some form of sticky reads work when E != Qw. Basically, we could make it use a subset of the bookies when doing reads. It wouldn't be completely sticky, but it would take better advantage of the read ahead cache.
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.
LGTM
e8be179
to
cb1070b
Compare
Co-authored-by: Michael Marshall <[email protected]>
Co-authored-by: Anonymitaet <[email protected]>
cb1070b
to
bd74506
Compare
Codecov Report
@@ Coverage Diff @@
## master #18003 +/- ##
=========================================
Coverage ? 45.72%
Complexity ? 17516
=========================================
Files ? 1573
Lines ? 128211
Branches ? 14098
=========================================
Hits ? 58626
Misses ? 63496
Partials ? 6089
Flags with carried forward coverage won't be shown. Click here to find out more. |
Motivation
Important details are missing for documenting ensemble (E) size, write quorum (Qw) size and ack quorum (Qa) size.
For example, it is not well known that there's a rule that E == Qw to use sticky reads or that the max number of bookie failures is Qa-1. Some might realize this only in production when they lose a bookie and cannot recover.
Modifications
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: