HBASE-22491 Separate the heap HFileBlock and offheap HFileBlock because the heap block won't need refCnt and save into prevBlocks list before shipping #268
Conversation
openinx
changed the title
|
💔 -1 overall
This message was automatically generated. |
| @@ -523,15 +523,15 @@ void updateCurrBlockRef(HFileBlock block) { | |||
| if (block != null && curBlock != null && block.getOffset() == curBlock.getOffset()) { | |||
| return; | |||
| } | |||
| if (this.curBlock != null) { | |||
| if (this.curBlock != null && !this.curBlock.isOnHeap()) { | |||
There was a problem hiding this comment.
We still have this but in a different way. Instead of MemType the block knows its type.
There was a problem hiding this comment.
Discussed with @anoopsjohn before, Maitaining a MemType inside the block will make the HFileBlock complex and easy to write a bug, because we need to handle the shared & non-shared logics at the same paths or methods.
Separating them into two classes will be more clear.
There was a problem hiding this comment.
Ya its ok we dont keep the memtype in single class type. But why we can not create 2 block types SharedMem vs ExclusiveMem rather than on heap vs off heap?
There was a problem hiding this comment.
Addressed this in the new patch. Thanks.
| @@ -763,7 +738,7 @@ public long heapSize() { | |||
| /** | |||
| * @return true to indicate the block is allocated from JVM heap, otherwise from off-heap. | |||
| */ | |||
| boolean isOnHeap() { | |||
| public boolean isOnHeap() { | |||
| return buf.hasArray(); | |||
There was a problem hiding this comment.
Nit : Just return 'false' here? Since you have HeapHfileBlock now where isOnheap() is always true?
There was a problem hiding this comment.
Think about this again, lots of UT use the HFileBlock with heap buf , if just return false here, I guess those UT will be failure.... so plan to keep this.
There was a problem hiding this comment.
Why cant do the other way around? Like new class type for the pooled BB backed blocks?
There was a problem hiding this comment.
Thought about this before, our ByteBuffAllocator won't allocate pooled heap BB , also if allocate a offheap BB, then it must be a pooled one. That's to say, heapBB = exclusiveBB, offheapBB = sharedBB. So if it's a heapBB, then must be a exclusiveBB..
There was a problem hiding this comment.
Think about this again, lots of UT use the HFileBlock with heap buf , if just return false here, I guess those UT will be failure.... so plan to keep this.
Then why cant you just do the reverse way? HFileBlock will be on heap one. So the UTs also dont need any change. We will have an extension to it. Name it like SharedMemHFileBlock or OffheapHFileBlock (I prefer the former). This will look a bit strange here where the HFileBlock as such is not telling whether it is on heap or off heap and we have a On heap specific extended class again!
There was a problem hiding this comment.
OK, let me create two classes: one named ExclusiveMemHFileBlock , the other one is SharedMemHFileBlock, will be easy to see the difference between those two kinds of blocks.
|
Just few nits and questions. Rest looks good to me. |
|
💔 -1 overall
This message was automatically generated. |
|
The two timeout UT TestFromClientSide3/TestFromClientSide are unrelated. Any other concerns ? @anoopsjohn @ramkrish86 |
|
Rebased HBASE-21879 branch with master branch , and resolve all conflicts now. Let's see what hadoop QA will say. |
| @@ -763,7 +738,7 @@ public long heapSize() { | |||
| /** | |||
| * @return true to indicate the block is allocated from JVM heap, otherwise from off-heap. | |||
| */ | |||
| boolean isOnHeap() { | |||
| public boolean isOnHeap() { | |||
| return buf.hasArray(); | |||
There was a problem hiding this comment.
Why cant do the other way around? Like new class type for the pooled BB backed blocks?
| @@ -523,15 +523,15 @@ void updateCurrBlockRef(HFileBlock block) { | |||
| if (block != null && curBlock != null && block.getOffset() == curBlock.getOffset()) { | |||
| return; | |||
| } | |||
| if (this.curBlock != null) { | |||
| if (this.curBlock != null && !this.curBlock.isOnHeap()) { | |||
There was a problem hiding this comment.
Ya its ok we dont keep the memtype in single class type. But why we can not create 2 block types SharedMem vs ExclusiveMem rather than on heap vs off heap?
| } | ||
|
|
||
| public HFileBlock build() { | ||
| if (buf.hasArray()) { |
There was a problem hiding this comment.
I still feel we should take any assumptions here. Better keep the notion of whether the block is backed by an exclusive or shared memory area. Based on that create different type of object. Here we always see whether on heap and if so exclusive memory. Why we have to drop that way?
There was a problem hiding this comment.
Why we have to drop that way?
You mean MemoryType ? It's a concept around all the IOEngine / BlockCache / ByteBuffAllocator /HFileBlock etc... Actually, as I commented above, heap or offheap can decide the shared or non-shared. Then all those MemoryType places can be uniformed in this build() method here, I'd prefer this simple implementation.
There was a problem hiding this comment.
Dont mean to keep the memory Type. That is not needed in HFileBlock. Even in the Builder too.. A setter which takes boolean to say whether it is exclusive memory or not would have been enough. Any way this assumption is true as of now. So its ok a continue this way if u strongly feel so
| // We don't have to keep ref to EXCLUSIVE type of block | ||
| if (this.curBlock != null) { | ||
| // We don't have to keep ref to heap block | ||
| if (this.curBlock != null && !this.curBlock.isOnHeap()) { |
There was a problem hiding this comment.
Ya here only difference is you will see whether the block is a Shared memory backed. If so add ref. We dont really worry whether it is off heap or on heap.
There was a problem hiding this comment.
Adding to prevBlocks is only at these 2 places?
There was a problem hiding this comment.
Adding to prevBlocks is only at these 2 places?
Yeah, check all the places, only the two add here.
|
|
||
| @Override | ||
| public HeapHFileBlock retain() { | ||
| // do nothing |
| @@ -373,7 +373,7 @@ private Cacheable asReferencedHeapBlock(Cacheable buf) { | |||
| if (buf instanceof HFileBlock) { | |||
| HFileBlock blk = ((HFileBlock) buf); | |||
| if (!blk.isOnHeap()) { | |||
There was a problem hiding this comment.
Here also it will be if shared memory type of block
There was a problem hiding this comment.
At least the API name in HFileBlock we can keep like isSharedMemory() instead of this on heap vs off heap? That will be more clear when reading parts of code like here. WHy we do this clone.
There was a problem hiding this comment.
At least the API name in HFileBlock we can keep like isSharedMemory() instead of this on heap vs off heap?
Will impl this in the new patch.
WHy we do this clone.
Because it's a offheap one, while our LRU only keep heap block, so need a deep clone here, please see: HBASE-22127
There was a problem hiding this comment.
That was not a Q why we clone. :-) My bad. I put a '.' in between. I mean with that name change and using that API in other code places like above, it will be more clear for any new reader to understand why we do this clone.
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
I still stand my point that this is not the most important thing for now. We should change the way that we release ByteBuffs, so we could solve the problem for both on-heap and off-heap ByteBuffs. And later we could see if we can optimize for on-heap ByteBuffs. In general, I think we will make use of off-heap ByteBuffs as much as possible so I do not think it worth to spend so much time to optimize for on-heap ByteBuffs right now...
|
🎊 +1 overall
This message was automatically generated. |
| .withNextBlockOnDiskSize(nextBlockOnDiskSize) | ||
| .withHFileContext(fileContext) | ||
| .withByteBuffAllocator(allocator) | ||
| .withOffset(offset) |
There was a problem hiding this comment.
Why withOffset() been called twice?
There was a problem hiding this comment.
Yeah, Let me fix this. Thanks.
|
💔 -1 overall
This message was automatically generated. |
|
Addressed the failed UT & checkstyle from HBaseQA. |
|
🎊 +1 overall
This message was automatically generated. |
| */ | ||
| boolean isOnHeap() { | ||
| return buf.hasArray(); | ||
| public boolean isSharedMem() { |
There was a problem hiding this comment.
Still we create HFileBlock only in tests? If not we could have left this API abstract only and even HFileBlock. That would have been best. May be in a follow up we can do that. Just renaming will be enough. In this jira keep it this way only. Fine.
| } | ||
|
|
||
| static HFileBlock shallowClone(HFileBlock blk) { | ||
| return createBuilder(blk).withShared(!blk.buf.hasArray()).build(); |
There was a problem hiding this comment.
blk.buf.hasArray() -> Instead use blk.isShared()
…se the heap block won't need refCnt and save into prevBlocks list before shipping
|
💔 -1 overall
This message was automatically generated. |
…se the heap block won't need refCnt and save into prevBlocks list before shipping (#268)
…se the heap block won't need refCnt and save into prevBlocks list before shipping (#268)
…se the heap block won't need refCnt and save into prevBlocks list before shipping (apache#268)
…se the heap block won't need refCnt and save into prevBlocks list before shipping (apache#268)
…se the heap block won't need refCnt and save into prevBlocks list before shipping