-
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-26488 Memory leak when MemStore retry flushing #3899
Conversation
🎊 +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. |
I think the intention here is that, for a memstore snapshot, it will never change, so we do not need to create a scanner for it every time, just reuse the same one. So here, I prefer we introduce a DelegateKeyValueScanner, which will delegate all the methods to the segment scanner, except the close method. And once the snapshot itself is closed, we will call the close method of the actual segment scanners. WDYT? Thanks. |
@Apache9 , thank you very much for review. In my opinion seems that introducing a
@Apache9 , that is all of my thought, hoping for your feedback. |
I do not think you fully got my point... My point is, the design of the MemStoreSnapshot is mainly for storing the snapshot id and the snapshot scanner, if we do not want to store the scanner in it, I think we could just remove this class, just call MemStore.getSnapshot every time when we want to use it... And for the problem 2, it is a problem, if it is a Closeable, then we should always close it. I checked the code, seems the only place where we miss the snapshot.close is when committing a flush, we should call close before calling updateStorefiles. And for problem 3, I do not see any difficulties to understand the logic? The design is that, the segment will not be changed any more, so we always use the same scanners to scan it. in order to not close it before we clear the snapshot, we should try to use DelegatingKeyValueScanner to implement an empty close method, and then close it in the MemStoreSnapshot.close method. This is an optimization, it will have more logics but I do not think it is too hard to understand. And for problem 4, as said above, this is an optimization, maybe it can not impact the performance a lot, but it does not introduce too much complexity too, so for me I think it is worth a try. There is a DelegatingKeyValueScanner implementation under the src/test, I think we can just move it src/main and use it. Thanks. |
@Apache9 ,thank you very much for detailed reply, yes, I understand your point, maybe I did not explain clearly. My opinion is that whether we could make the code more simpler? Whether we could make the And for problem 3, I did not mean btw. And for the problem 2,seems that if the flush failed,because @Apache9 ,in short, my opinion is just to make the code somewhat more simpler,we could remove |
Fine, then let's just remove the close method of MemStoreSnapshot since it is useless now? And also change the javadoc to reflect the new design and implementation. |
@Apache9 , thank you very much for feedback, removing the close method of |
🎊 +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. |
Signed-off-by: Duo Zhang <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit e20b2f3) Change-Id: Iffe855b34b3e667321c6a3550e5d59036ba7a8ca
No description provided.