HBASE-30246 MOB compaction does not close MobCell after resolving ref…#8392
HBASE-30246 MOB compaction does not close MobCell after resolving ref…#8392liuxiaocs7 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR addresses HBASE-30246 by ensuring MOB reference resolution during compaction properly closes the MobCell and returns a safe, heap-resident copy of the resolved cell to avoid leaking scanners/buffers and referencing recycled NIO buffers.
Changes:
- Introduce
DefaultMobStoreCompactor#resolveMobCell(...)that closesMobCelland returns an independent copiedExtendedCell. - Update compaction paths to use
resolveMobCell(...)instead of directly usingmobStore.resolve(...).getCell(). - Strengthen tests to use try-with-resources for
MobCelland add a unit test verifying close + copy semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hbase-server/src/main/java/org/apache/hadoop/hbase/mob/DefaultMobStoreCompactor.java | Adds resolveMobCell helper to close resolved MobCell and copy resolved cells safely; compaction updated to use it. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/mob/FaultyMobStoreCompactor.java | Aligns test compactor implementation with new resolveMobCell behavior. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/mob/TestDefaultMobStoreCompactor.java | Adds unit test verifying resolveMobCell closes MobCell and returns an independent copy. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHMobStore.java | Updates resolve test to close MobCell via try-with-resources and dereference via getCell(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * references. | ||
| */ | ||
| protected ExtendedCell resolveMobCell(ExtendedCell reference) throws IOException { | ||
| try (MobCell mobCell = mobStore.resolve(reference, cacheMobBlocksOnCompaction, false)) { |
There was a problem hiding this comment.
Maybe protected ExtendedCell resolveMobCell(ExtendedCell reference, boolean flag) is better?
I think we can also reuse this method in the future if we need to call mobStore.resolve(reference, cacheMobBlocksOnCompaction, true) by this way
There was a problem hiding this comment.
Thanks @guluo2016 ! The false (readEmptyValueOnMobCellMiss) is coupled to this path's miss handling (it relies on resolve throwing, while true returns an empty cell needing different handling), so it can't be reused as-is. I'd prefer to add the param with a proper name once a real true caller appears, WDYT?
…erence cells