-
Notifications
You must be signed in to change notification settings - Fork 3.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
HBASE-28401 Introduce a close method for memstore for release active … #5705
Conversation
Let's see if this could fix the LEAK problem. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
With this patch in place, there is no leak report in TestHRegion's log output now. I think we can apply this first, and then check whether there are still memory leak report in ITBLL runs. |
public void close() { | ||
// active should never be null | ||
active.close(); | ||
// for snapshot, either it is empty, where we do not reference any real segment which contains a |
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.
I'm of the opinion that we should always close all closeables. I don't dispute your comment here, but I feel like we should close the snapshot either way. This prevents future bugs where assumptions might be made about the presence of a close() method on ImmutableSegment.
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.
I was thinking the same way when implementing this patch but it is not easy to get things right. It requires a big refactoring, on how we do ref counting here. So I prefer we fix the problem first to see there are still other leaks, and open another issue for refactoring. The latter one will take some time, since the code is not easy to fully understand...
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.
Filed HBASE-28435 for revisiting the referenc counting way. Do you have any other concerns? @bbeaudreault
…segment (apache#5705) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3b00db0)
…segment (#5705) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3b00db0)
…segment (#5705) (#5761) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3b00db0)
…segment (#5705) (#5761) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3b00db0)
…segment (#5705) (#5761) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3b00db0)
Sorry for being late here, this looks good overall. Just to confirm, this patch is sufficient for getting rid of memstore leak logs or we still need HBASE-28435? IIUC, HBASE-28435 is more of good to have right? |
This patch is enough to solve one possible memory leak log problem(although actually there is no real memory leak). HBASE-28435 is for refactoring, and yes, it is a good to have. |
…segment