Anoop Sam John commented on HBASE-16643:

Not sure for the bug fix we need this big change.
We can just remove below line and that will fix the bug
It is ok also.. As we dont really closing the scanners here..  We just replace 
the container KVHeap with a ReverseKVHeap with same all SegmentScanners.  Pls 
consider fix that way?
The reverse scan info was not passed from StoreScanner level onwards.   If u 
strongly feel we need some refactor and pass the reverse type throughout, 
suggest fix in another IA issue.  There can be some other improvements also 
possible there.
BTW just noticed that methods in MemstoreScanner is made synchronized.  I dont 
think we really need it.   That also can be avoided in that IA issue.

Pls add some comments why we dont close  forwardHeap but just nullify it.

> Reverse scanner heap creation may not allow MSLAB closure due to improper ref 
> counting of segments
> --------------------------------------------------------------------------------------------------
>                 Key: HBASE-16643
>                 URL: https://issues.apache.org/jira/browse/HBASE-16643
>             Project: HBase
>          Issue Type: Bug
>            Reporter: ramkrishna.s.vasudevan
>            Assignee: ramkrishna.s.vasudevan
>            Priority: Critical
>             Fix For: 2.0.0
>         Attachments: HBASE-16643.patch, HBASE-16643_1.patch, 
> HBASE-16643_2.patch
> In the reverse scanner case,
> While doing 'initBackwardHeapIfNeeded' in MemstoreScanner for setting the 
> backward heap, we do a 
> {code}
> if ((backwardHeap == null) && (forwardHeap != null)) {
>         forwardHeap.close();
>         forwardHeap = null;
>         // before building the heap seek for the relevant key on the scanners,
>         // for the heap to be built from the scanners correctly
>         for (KeyValueScanner scan : scanners) {
>           if (toLast) {
>             res |= scan.seekToLastRow();
>           } else {
>             res |= scan.backwardSeek(cell);
>           }
>         }
> {code}
> forwardHeap.close(). This would internally decrement the MSLAB ref counter 
> for the current active segment and snapshot segment.
> When the scan is actually closed again we do close() and that will again 
> decrement the count. Here chances are there that the count would go negative 
> and hence the actual MSLAB closure that checks for refCount==0 will fail. 
> Apart from this, when the refCount becomes 0 after the firstClose if any 
> other thread requests to close the segment, then we will end up in corrupted 
> segment because the segment could be put back to the MSLAB pool. 

This message was sent by Atlassian JIRA

Reply via email to