-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
ZOOKEEPER-4794: Reduce the ZKDatabase#committedLog memory usage. #2115
ZOOKEEPER-4794: Reduce the ZKDatabase#committedLog memory usage. #2115
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.
Overall I appreciate the refactor, the code looks nicer.
But I am not sure about the impact of the change, maybe we are solving one problem but we add another problem somewhere else.
I have added a comment, PTAL
public byte[] getSerializeData() { | ||
if (this.hdr == null) { | ||
return null; | ||
} | ||
|
||
if (this.serializeData == null) { |
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.
so basically you are saying that caching this value is useless ?
do you know why we were caching it ?
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.
See #2030, the aim is to reduce the serialization. From the prd, the memory looks more sensitive.
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
thanks for your clarification
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. Thanks @horizonzy !
// This is the only place that can throw IO exception | ||
hasNext = itr.next(); | ||
|
||
} catch (IOException e) { | ||
LOG.error("Unable to read txnlog from disk", e); | ||
hasNext = false; | ||
p = new Proposal(); |
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.
Weird to create objects in catch block, but it's okay.
Merged to |
horizonzy |
Thanks, now you're officially a ZooKeeper contributor. ;) |
Reduce the committed log memory usage. Fix ci. Reviewers: eolivelli, hangc0276, anmolnar Author: horizonzy Closes apache#2115 from horizonzy/reduce-committed-log-memory-usage
In most cases, the committed log isn't used, it's only used in the fast follower synchronization.
So we can generate the QuorumPacket when fast follower synchronizing to reduce the ZKDatabase#committedLog memory usage.