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.

Sign up for GitHub

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

Jump to bottom

Ledger length is not updated correctly in LedgerHandleAdv #683

Closed
sijie opened this issue Oct 31, 2017 · 6 comments
Closed

Ledger length is not updated correctly in LedgerHandleAdv #683

sijie opened this issue Oct 31, 2017 · 6 comments
Labels
area/client priority/blocker release/4.5.1 type/bug

Comments

@sijie
Copy link
Member

sijie commented Oct 31, 2017 

ivankelly [4:37 AM] 
@eolivelli the adv ledger stuff, does it take the ledger length into account?


[4:37] 
when adding entries


eolivelli [8:30 AM] 
I hope so


[8:30] 
I am using LEdgerHAndleAdv and it is working


[8:30] 
I have to check code


[8:31] 
yes


[8:31] 
see doAsyncAddEntry in LedgerHandleAdv


[8:31] 
https://github.com/apache/bookkeeper/blob/c6ca4ecbf76f6dac7ebcd843a429dbbaaba62420/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandleAdv.java#L182
GitHub
apache/bookkeeper
bookkeeper - Apache Bookkeeper
 


ivankelly [10:25 AM] 
yes, that's what I'm asking about


[10:25] 
It's basically doubling the length of the ledger each time, without taking the data length into consideration


sijie [10:49 AM] 
@ivankelly I think this is a problem introduced by  netty 4 upgrade


[10:50] 
before netty 4 upgrade, the `length` is the entry length.


[10:52] 
the problem was introduced at 7b1eec47092d0de6776c5a89575dbfc678165ee7
@sijie sijie added this to the 4.6.0 milestone Oct 31, 2017
@sijie
Copy link
Member Author

sijie commented Oct 31, 2017

The problem was introduced when we use ByteBuf in 7b1eec4

@sijie
Copy link
Member Author

sijie commented Oct 31, 2017

@eolivelli since you haven't cut the 4.5.1 tag, it would be great that we include this in release/4.5.1 as well.

@eolivelli
Copy link
Contributor

Ok

@sijie
Copy link
Member Author

sijie commented Oct 31, 2017

@ivankelly has fixed this issue at #664. that will be for master.

@ivankelly it would be great if you can do a similar fix (without the recycler stuffs) for branch 4.5.1.

@sijie sijie removed this from the 4.6.0 milestone Oct 31, 2017
@sijie
Copy link
Member Author

sijie commented Oct 31, 2017

removed this from release/4.6.0 and tag it for release/4.5.1 only.

ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Nov 2, 2017
@ivankelly
ISSUE apache#683: Correctly update length in LedgerHandleAdv
8fd2199 
Commit 7b1eec introduced a change which removed the length parameter
to doAsyncAddEntry. This resulted in the length of the ledger being
added to itself each time for a LedgerHandleAdv, which ultimately
meant that that length would be 0, as it never had anything non-zero
added to it.

This change corrects this, by adding the length of the data parameter
to the ledger length.
eolivelli pushed a commit that referenced this issue Nov 2, 2017
@ivankelly @eolivelli
ISSUE #683: Correctly update length in LedgerHandleAdv
c3fe58d 
Commit 7b1eec introduced a change which removed the length parameter
to doAsyncAddEntry. This resulted in the length of the ledger being
added to itself each time for a LedgerHandleAdv, which ultimately
meant that that length would be 0, as it never had anything non-zero
added to it.

This change corrects this, by adding the length of the data parameter
to the ledger length.

Author: Ivan Kelly <ivank@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>

This closes #684 from ivankelly/len-fix, closes #683
@eolivelli
Copy link
Contributor

Patch merged to branch-4.5

@sijie sijie mentioned this issue Nov 2, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/client priority/blocker release/4.5.1 type/bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sijie @eolivelli