comnetwork edited a comment on pull request #3899: URL: https://github.com/apache/hbase/pull/3899#issuecomment-986219358
@Apache9 , thank you very much for review. In my opinion seems that introducing a `DelegateKeyValueScanner` could not solve the problem: 1. It seems a reference count problem , the `SnapshotSegmentScanner` seems independent of whether snapshot segment itself is closed or not, even if snapshot segment is closed, if there is `SnapshotSegmentScanner` or `SegmentScanner` is using，we could not release the `MemStoreLAB` of the snapshot segment. 2. Mostly，I find that `MemStoreSnapshot.close` is not always invoked under every situation, eg. flushing success or failed does not invoke `MemStoreSnapshot.close` in fact, so we could not depend on` MemStoreSnapshot.close` to close the `SnapshotSegmentScanner` and I think we could remove the `MemStoreSnapshot.close `in the future to simplify the logic. 3. and I think that introducing another `DelegateKeyValueScanner` for wrapping `SnapshotSegmentScanner` is somewhat complex, because `SnapshotSegmentScanner` is only used for `MemStoreSnapshot `here, so seems that we create another additional wrapper for this `SnapshotSegmentScanner` is somewhat complex and unnecessary because the lazy close logic overlaying reference count logic is hard to understand and error-prone, just use `SnapshotSegmentScanner` for reference count logic is clear and simple. 4. The overhead of creating a new `SnapshotSegmentScanner` every time when `MemStoreSnapshot.getScanners` is called is low, just as the behavior of `DefaultMemStore.getScanners `and `Segment.getScanners`.The key in the `SnapshotSegmentScanner` ctor is to increase the reference count of the `MemStoreLAB` of the snapshot segment. @Apache9 , that is all of my thought, hope for your 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
