-
Notifications
You must be signed in to change notification settings - Fork 903
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
LedgerEntry#getLength does not do what the documentation says #1411
Comments
good catch |
for BC consideration, we can fix the documentation first. We can consider deprecating
|
there shouldn't be a BC consideration for the new API |
@ivankelly okay you are talking about the new API. for new API it is fine. I was talking about the old API. http://bookkeeper.apache.org/docs/latest/api/javadoc/org/apache/bookkeeper/client/LedgerEntry.html#getLength-- |
Ya, old API can stay as it is. Maybe deprecate it. I seem to remember it being very broken at some point in any case, and noone complained. |
+1 for fixing only the new API and deprecate old API method |
why? it is not broken, we used that at twitter to know how many bytes a reader is lagging behind a writer, by comparing the lengths of two entries. |
let's submit a BP and fix the new API and deprecate the method in the old API. |
@sijie This one (https://issues.apache.org/jira/browse/BOOKKEEPER-673). In my head it was more recent than almost 5 years ago. Getting old I guess. |
@ivankelly I see. but that's more about consistency between metadata and the information in entries. I don't know a direct usage of length in metadata, but the information in entries is more useful. |
Pulsar tiered storage usage uses the length in the metadata. |
I've also encountered this problem in a BookKeeper test. It seems to still exist in the latest version of BookKeeper. Currently, the |
You would expect getLength on a LedgerEntry to return the length of the data in the entry. The documentation says it does this, and the Mock implementation do so too. But this is not what it does. It in fact returns the length of the ledger up to and including this entry.
At the very least this is a documentation bug, but this name is super confusing even if the documentation is updated.
The text was updated successfully, but these errors were encountered: