comnetwork edited a comment on pull request #3899: URL: https://github.com/apache/hbase/pull/3899#issuecomment-993129730
@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 `MemStoreSnapshot` just a `Context` Object which just as branch-1 does, not a `Closeable` object(we could remove `Closeable` from `MemStoreSnapshot`)？By creating a new `SnapshotSegmentScanner` every time when `MemStoreSnapshot.getScanners` is called we could avoid forgetful `MemStoreSnapshot.close` and could also avoid using a `DelegatingKeyValueScanner` to delay close. And for problem 3, I did not mean `DelegatingKeyValueScanner` itself is hard to understand, I mean that on the one hand we using delay close `SnapshotSegmentScanner` in `DelegatingKeyValueScanner` and on the other hand `SnapshotSegmentScanner` itself also has reference count, so these two factors make it more indirect when we want to locate or track some memory problem about the segment and this indirect could avoid. If there is no reference count for `SnapshotSegmentScanner`, I think using `DelegatingKeyValueScanner` to delay close is a better approach. btw. And for the problem 2，seems that if the flush failed，because `SnapshotSegmentScanner.close` is called, seems that `MemStoreSnapshot.close` is also not called. @Apache9 ,in short, my opinion is just to make the code somewhat more simpler，hoping for your further feedback. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
