Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      From 2.0 branch onwards KVComparator and its subclasses MetaComparator, RawBytesComparator are all deprecated.
      All the comparators are moved to CellComparator. MetaCellComparator, a subclass of CellComparator, will be used to compare hbase:meta cells.
      Previously exposed static instances KeyValue.COMPARATOR, KeyValue.META_COMPARATOR and KeyValue.RAW_COMPARATOR are deprecated instead use CellComparator.COMPARATOR and CellComparator.META_COMPARATOR.
      Also note that there will be no RawBytesComparator. Where ever we need to compare raw bytes use Bytes.BYTES_RAWCOMPARATOR.
      CellComparator will always operate on cells and its components, abstracting the fact that a cell can be backed by a single byte[] as opposed to how KVComparators were working.
      Show
      From 2.0 branch onwards KVComparator and its subclasses MetaComparator, RawBytesComparator are all deprecated. All the comparators are moved to CellComparator. MetaCellComparator, a subclass of CellComparator, will be used to compare hbase:meta cells. Previously exposed static instances KeyValue.COMPARATOR, KeyValue.META_COMPARATOR and KeyValue.RAW_COMPARATOR are deprecated instead use CellComparator.COMPARATOR and CellComparator.META_COMPARATOR. Also note that there will be no RawBytesComparator. Where ever we need to compare raw bytes use Bytes.BYTES_RAWCOMPARATOR. CellComparator will always operate on cells and its components, abstracting the fact that a cell can be backed by a single byte[] as opposed to how KVComparators were working.
    1. HBASE-10800_7.patch
      356 kB
      ramkrishna.s.vasudevan
    2. HBASE-10800_6.patch
      272 kB
      ramkrishna.s.vasudevan
    3. HBASE-10800_5.patch
      268 kB
      ramkrishna.s.vasudevan
    4. HBASE-10800_4.patch
      273 kB
      ramkrishna.s.vasudevan
    5. HBASE-10800_4.patch
      273 kB
      ramkrishna.s.vasudevan
    6. HBASE-10800_3.patch
      274 kB
      ramkrishna.s.vasudevan
    7. HBASE-10800_29.patch
      378 kB
      ramkrishna.s.vasudevan
    8. HBASE-10800_28.patch
      378 kB
      ramkrishna.s.vasudevan
    9. HBASE-10800_27.patch
      378 kB
      ramkrishna.s.vasudevan
    10. HBASE-10800_26.patch
      379 kB
      ramkrishna.s.vasudevan
    11. HBASE-10800_25.patch
      378 kB
      ramkrishna.s.vasudevan
    12. HBASE-10800_24.patch
      366 kB
      ramkrishna.s.vasudevan
    13. HBASE-10800_23.patch
      364 kB
      ramkrishna.s.vasudevan
    14. HBASE-10800_23.patch
      364 kB
      ramkrishna.s.vasudevan
    15. HBASE-10800_2.patch
      201 kB
      ramkrishna.s.vasudevan
    16. HBASE-10800_18.patch
      389 kB
      ramkrishna.s.vasudevan
    17. HBASE-10800_16.patch
      388 kB
      ramkrishna.s.vasudevan
    18. HBASE-10800_15.patch
      387 kB
      ramkrishna.s.vasudevan
    19. HBASE-10800_14.patch
      387 kB
      ramkrishna.s.vasudevan
    20. HBASE-10800_13.patch
      388 kB
      ramkrishna.s.vasudevan
    21. HBASE-10800_12.patch
      392 kB
      ramkrishna.s.vasudevan
    22. HBASE-10800_11.patch
      391 kB
      ramkrishna.s.vasudevan
    23. HBASE-10800_1.patch
      177 kB
      ramkrishna.s.vasudevan

      Issue Links

        Activity

        Hide
        ramkrishna.s.vasudevan added a comment -

        Changes KVComparator to implement CellComparator. All core methods are in CellComparator. MetaComparator is still in KV because it is handling special cases. All changes done in the read path mostly.
        Removed some of the methods from KeyValue and moved to KeyValueUtil. So KeyValue class is cleaned up.
        SamePrefixCompartor is removed as it was not getting used except in testcase. Still running testcases, most of them are passing.
        This would help in adding comparator methods in the CellComparator that could even compare offheap bytes. Will it be better to make CellComparator an interface? Thoughts?

        Show
        ramkrishna.s.vasudevan added a comment - Changes KVComparator to implement CellComparator. All core methods are in CellComparator. MetaComparator is still in KV because it is handling special cases. All changes done in the read path mostly. Removed some of the methods from KeyValue and moved to KeyValueUtil. So KeyValue class is cleaned up. SamePrefixCompartor is removed as it was not getting used except in testcase. Still running testcases, most of them are passing. This would help in adding comparator methods in the CellComparator that could even compare offheap bytes. Will it be better to make CellComparator an interface? Thoughts?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12653437/HBASE-10800_1.patch
        against trunk revision .
        ATTACHMENT ID: 12653437

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 69 new or modified tests.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 findbugs. The patch appears to introduce 14 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + comparisonResult = CellComparator.CELL_COMPARATOR.compareRows(left, loffset + KeyValue.ROW_LENGTH_SIZE,
        + comp = samePrefixComparator.compareCommonRowPrefix(seekCell, currentCell, rowCommonPrefix);
        + public EncodedSeeker createSeeker(CellComparator comparator, HFileBlockDecodingContext decodingCtx) {
        + conf, cacheConf, (KVComparator)comparator, bloomType, maxKeyCount, favoredNodes, fileContext);

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestKeyValue

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hdfs.server.namenode.TestBackupNode.testCheckpoint(TestBackupNode.java:360)
        at org.apache.hadoop.hdfs.server.namenode.TestBackupNode.testCheckpointNode(TestBackupNode.java:134)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12653437/HBASE-10800_1.patch against trunk revision . ATTACHMENT ID: 12653437 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 69 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 findbugs . The patch appears to introduce 14 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + comparisonResult = CellComparator.CELL_COMPARATOR.compareRows(left, loffset + KeyValue.ROW_LENGTH_SIZE, + comp = samePrefixComparator.compareCommonRowPrefix(seekCell, currentCell, rowCommonPrefix); + public EncodedSeeker createSeeker(CellComparator comparator, HFileBlockDecodingContext decodingCtx) { + conf, cacheConf, (KVComparator)comparator, bloomType, maxKeyCount, favoredNodes, fileContext); +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestKeyValue -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hdfs.server.namenode.TestBackupNode.testCheckpoint(TestBackupNode.java:360) at org.apache.hadoop.hdfs.server.namenode.TestBackupNode.testCheckpointNode(TestBackupNode.java:134) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9924//console This message is automatically generated.
        Hide
        stack added a comment -

        It is not you but what is compareStatic? (when the method is not static)

        Why not compareKey rather than compareOnlyKeyPortion?

        Why we have to take a flag on compareStatic when Key only? Should there be a compareKey only method that is called when you are comparing the whole Cell? (Do we ever compare Value? If not, why we have to say compare Key at all?)

        Should this be a method on its own?

        + int c = Bytes.compareTo(a.getRowArray(), a.getRowOffset(), a.getRowLength(),
        + b.getRowArray(), b.getRowOffset(), b.getRowLength());

        My sense is that it is done lots of times throughout the codebase. Hmm... doesn't compareRows do the above? Call lit?

        Why you have non-static methods? You ask if should be an Interface. You thinking different CellComparators? I'd think that it'd be the Cell that would change implementation. Would be trouble if the CellComparator itself changed... If we come up w/ faster CellComparator lets just use it in place of the old? Or what you thinking Ram?

        I've asked before but forgot, what is a flat key? compareFlatKey

        Should CellComparator only have compareTo in it? And then CellUtil has the rest? Maybe KVComparator didn't do this so you have CellComparator not doing it. MatchingRow was in KVComparator? Should it be in CellUtil? Hmm... seems like you have put a bunch of the compares into CellUtil. Good. Make the separation cleaner?

        Feels like these methods should be static again Ram.

        If you can get away with purging that much from KeyValue, more power to you!

        Thanks for digging in on this Ram.

        Show
        stack added a comment - It is not you but what is compareStatic? (when the method is not static) Why not compareKey rather than compareOnlyKeyPortion? Why we have to take a flag on compareStatic when Key only? Should there be a compareKey only method that is called when you are comparing the whole Cell? (Do we ever compare Value? If not, why we have to say compare Key at all?) Should this be a method on its own? + int c = Bytes.compareTo(a.getRowArray(), a.getRowOffset(), a.getRowLength(), + b.getRowArray(), b.getRowOffset(), b.getRowLength()); My sense is that it is done lots of times throughout the codebase. Hmm... doesn't compareRows do the above? Call lit? Why you have non-static methods? You ask if should be an Interface. You thinking different CellComparators? I'd think that it'd be the Cell that would change implementation. Would be trouble if the CellComparator itself changed... If we come up w/ faster CellComparator lets just use it in place of the old? Or what you thinking Ram? I've asked before but forgot, what is a flat key? compareFlatKey Should CellComparator only have compareTo in it? And then CellUtil has the rest? Maybe KVComparator didn't do this so you have CellComparator not doing it. MatchingRow was in KVComparator? Should it be in CellUtil? Hmm... seems like you have put a bunch of the compares into CellUtil. Good. Make the separation cleaner? Feels like these methods should be static again Ram. If you can get away with purging that much from KeyValue, more power to you! Thanks for digging in on this Ram.
        Hide
        ramkrishna.s.vasudevan added a comment -

        It is not you but what is compareStatic? (when the method is not static)

        Okie. Can change this to just compare.

        Why not compareKey rather than compareOnlyKeyPortion?

        The reason for saying compareOnlyKeyPortion is because after the introduction of replay tags in certain cases we need to compare that and also the mvcc part of the key. (before flushing and compaction).
        Hence this compareOnlyKeyPortion.

        Why we have to take a flag on compareStatic when Key only?

        They flag is to denote the types of compare that we may have to do as per the above comment. (Ignore or include replay tags and mvcc).

        Hmm... doesn't compareRows do the above? Call lit?

        Changed such that compareRows is used.

        Would be trouble if the CellComparator itself changed

        In many places we have an API called getKVComparator. Now if we have different types of Cells and each cell has its own cellcomparator still when we say getComparator() we should be returning the Cell's comparator which is of type CellComparator? Or we should totally remove those comparators getting passed as method params or these getKVComparator and just use CellComparator.compare in static mode. That would also involve lot of places to change. In the current patch that part of the code is not changed.

        I've asked before but forgot, what is a flat key? compareFlatKey

        Comparing only the keyportion. This could be replaced with compare(Cell a, Cell b).Updated the patch. But left the compareFlatKey that accepts the byte[] as params.

        Should CellComparator only have compareTo in it? And then CellUtil has the rest?

        Same reasoning as above comment. Wanted to have as instance methods and whereever could be accessed in static way move them to the CellUtil.

        Take the case of MetaKeyComparator. There we are creating MetaKeyComparator as a subclass of KVComparator.
        Suppose the reader v2 and v3 tries to do a comparison on two cells which are offheap then the cell comparator used by that class would be the one which operates on offheap and should be specific to that and the related areas. There I thought subclassing CellComparator would be helpful.

        Show
        ramkrishna.s.vasudevan added a comment - It is not you but what is compareStatic? (when the method is not static) Okie. Can change this to just compare. Why not compareKey rather than compareOnlyKeyPortion? The reason for saying compareOnlyKeyPortion is because after the introduction of replay tags in certain cases we need to compare that and also the mvcc part of the key. (before flushing and compaction). Hence this compareOnlyKeyPortion. Why we have to take a flag on compareStatic when Key only? They flag is to denote the types of compare that we may have to do as per the above comment. (Ignore or include replay tags and mvcc). Hmm... doesn't compareRows do the above? Call lit? Changed such that compareRows is used. Would be trouble if the CellComparator itself changed In many places we have an API called getKVComparator. Now if we have different types of Cells and each cell has its own cellcomparator still when we say getComparator() we should be returning the Cell's comparator which is of type CellComparator? Or we should totally remove those comparators getting passed as method params or these getKVComparator and just use CellComparator.compare in static mode. That would also involve lot of places to change. In the current patch that part of the code is not changed. I've asked before but forgot, what is a flat key? compareFlatKey Comparing only the keyportion. This could be replaced with compare(Cell a, Cell b).Updated the patch. But left the compareFlatKey that accepts the byte[] as params. Should CellComparator only have compareTo in it? And then CellUtil has the rest? Same reasoning as above comment. Wanted to have as instance methods and whereever could be accessed in static way move them to the CellUtil. Take the case of MetaKeyComparator. There we are creating MetaKeyComparator as a subclass of KVComparator. Suppose the reader v2 and v3 tries to do a comparison on two cells which are offheap then the cell comparator used by that class would be the one which operates on offheap and should be specific to that and the related areas. There I thought subclassing CellComparator would be helpful.
        Hide
        stack added a comment -

        Hence this compareOnlyKeyPortion.

        But tags and mvcc are part of key? No?

        They flag is to denote the types of compare that we may have to do as per the above comment.

        To be clear, the flag is a little ugly. A method that spells out what the compare does would be better than a single method differentiated by a boolean flag IMO.

        Thanks for working on this ramkrishna.s.vasudevan This is an important one to get behind us.

        Show
        stack added a comment - Hence this compareOnlyKeyPortion. But tags and mvcc are part of key? No? They flag is to denote the types of compare that we may have to do as per the above comment. To be clear, the flag is a little ugly. A method that spells out what the compare does would be better than a single method differentiated by a boolean flag IMO. Thanks for working on this ramkrishna.s.vasudevan This is an important one to get behind us.
        Hide
        ramkrishna.s.vasudevan added a comment -

        >>But tags and mvcc are part of key? No?
        Tags are not part of key. mvcc it is not considered always. When we compare kvs that are already flushed/compacted then mvcc is not considered.

        To be clear, the flag is a little ugly. A method that spells out what the compare does would be better than a single method differentiated by a boolean flag IMO.

        Sure. Will make it cleaner.

        Show
        ramkrishna.s.vasudevan added a comment - >>But tags and mvcc are part of key? No? Tags are not part of key. mvcc it is not considered always. When we compare kvs that are already flushed/compacted then mvcc is not considered. To be clear, the flag is a little ugly. A method that spells out what the compare does would be better than a single method differentiated by a boolean flag IMO. Sure. Will make it cleaner.
        Hide
        ramkrishna.s.vasudevan added a comment -

        I will update this patch and make things work with Cell comparator.

        Show
        ramkrishna.s.vasudevan added a comment - I will update this patch and make things work with Cell comparator.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Latest patch. There are few places in the actual code where a comparator of type KV is passed. It is better to remove them. Also in BDE there is a cell comparator and a sameprefix comparator. Ideally both are same. But I would remove it in the patch HBASE-11895.
        Moved most of the methods in the Cellcomparator to CellUtil. The only thing that we need to decide now is incase we want to compare cells offheap should we add those compare methods also in the CellUtil? Think that would be enough. Will proceed with the next patch after this goes in.

        Show
        ramkrishna.s.vasudevan added a comment - Latest patch. There are few places in the actual code where a comparator of type KV is passed. It is better to remove them. Also in BDE there is a cell comparator and a sameprefix comparator. Ideally both are same. But I would remove it in the patch HBASE-11895 . Moved most of the methods in the Cellcomparator to CellUtil. The only thing that we need to decide now is incase we want to compare cells offheap should we add those compare methods also in the CellUtil? Think that would be enough. Will proceed with the next patch after this goes in.
        Hide
        ramkrishna.s.vasudevan added a comment -
        Show
        ramkrishna.s.vasudevan added a comment - https://reviews.apache.org/r/25423/ -RB link.
        Hide
        ramkrishna.s.vasudevan added a comment -

        To be clear, the flag is a little ugly. A method that spells out what the compare does would be better than a single method differentiated by a boolean flag IMO.

        This is also removed now. There are no boolean flags.
        compareFlatKey() name is not changed as it as references in the code. If we all agree we can rename that method. Here flat key is the part of the key that includes row, famly, qualifier and ts. It does not include the mvcc and tags.

        Show
        ramkrishna.s.vasudevan added a comment - To be clear, the flag is a little ugly. A method that spells out what the compare does would be better than a single method differentiated by a boolean flag IMO. This is also removed now. There are no boolean flags. compareFlatKey() name is not changed as it as references in the code. If we all agree we can rename that method. Here flat key is the part of the key that includes row, famly, qualifier and ts. It does not include the mvcc and tags.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12667129/HBASE-10800_2.patch
        against trunk revision .
        ATTACHMENT ID: 12667129

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 95 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10738//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12667129/HBASE-10800_2.patch against trunk revision . ATTACHMENT ID: 12667129 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 95 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10738//console This message is automatically generated.
        Hide
        Duo Zhang added a comment -

        Do we still work on this?
        I skimmed off-heap related issues, seems this issue could be a start point since it does not depend on any other issues...

        Show
        Duo Zhang added a comment - Do we still work on this? I skimmed off-heap related issues, seems this issue could be a start point since it does not depend on any other issues...
        Hide
        stack added a comment -

        Question for you ramkrishna.s.vasudevan

        Show
        stack added a comment - Question for you ramkrishna.s.vasudevan
        Hide
        Apache9 added a comment -

        So is HBASE-10191 still active or we are waiting for a new GC to solve the problem?
        Thanks~

        Show
        Apache9 added a comment - So is HBASE-10191 still active or we are waiting for a new GC to solve the problem? Thanks~
        Hide
        ramkrishna.s.vasudevan added a comment -

        Thanks for the interest over this JIRA and sorry for the delayed update on this. Missed the ping over here.
        The idea of this JIRA was to make Cells usage everywhere including the comparators. I have now deferred this activity because if you see in KVcomparator and its usage through out the code it is using Cells to compare. So now changing KVComparator to CellComparator is only a name change in most of the places. But still there are this Bloom filter and index usages that uses RawComparators which is not based on Cell.

        For the current work for offheap HBASE-11425 we have introduced new APIS that work on ByteBuffers and some new comparator APIs that work on BBs. Once that is in place we could move all the Comparators to Cell based and then work on this JIRA actively.
        Any comments/suggestions are welcome.

        Show
        ramkrishna.s.vasudevan added a comment - Thanks for the interest over this JIRA and sorry for the delayed update on this. Missed the ping over here. The idea of this JIRA was to make Cells usage everywhere including the comparators. I have now deferred this activity because if you see in KVcomparator and its usage through out the code it is using Cells to compare. So now changing KVComparator to CellComparator is only a name change in most of the places. But still there are this Bloom filter and index usages that uses RawComparators which is not based on Cell. For the current work for offheap HBASE-11425 we have introduced new APIS that work on ByteBuffers and some new comparator APIs that work on BBs. Once that is in place we could move all the Comparators to Cell based and then work on this JIRA actively. Any comments/suggestions are welcome.
        Hide
        Duo Zhang added a comment -
        {quote For the current work for offheap HBASE-11425 we have introduced new APIS that work on ByteBuffers and some new comparator APIs that work on BBs. Once that is in place we could move all the Comparators to Cell based and then work on this JIRA actively.}

        ByteBuffer or ByteRange? I see some issues such as HBASE-10772 aim to use BR instead of BB, but some issues still use BB?

        Show
        Duo Zhang added a comment - {quote For the current work for offheap HBASE-11425 we have introduced new APIS that work on ByteBuffers and some new comparator APIs that work on BBs. Once that is in place we could move all the Comparators to Cell based and then work on this JIRA actively.} ByteBuffer or ByteRange? I see some issues such as HBASE-10772 aim to use BR instead of BB, but some issues still use BB?
        Hide
        ramkrishna.s.vasudevan added a comment -

        Actually we have not yet published our findings on this. But we hope to do so this week.
        If you see the SocketChannel's in the RPC layer it is BB that SocketChannel's work with. So having BR is nothing but going to be an indirection over the BB. So considering that a BR wrapping BB is less preferred to directly using a BB.

        Show
        ramkrishna.s.vasudevan added a comment - Actually we have not yet published our findings on this. But we hope to do so this week. If you see the SocketChannel's in the RPC layer it is BB that SocketChannel's work with. So having BR is nothing but going to be an indirection over the BB. So considering that a BR wrapping BB is less preferred to directly using a BB.
        Hide
        Duo Zhang added a comment -

        So we decide to get rid of ByteRange?

        Show
        Duo Zhang added a comment - So we decide to get rid of ByteRange?
        Hide
        ramkrishna.s.vasudevan added a comment -

        Attaching a rebased patch with some additional changes (addresses some of the review comments posted in RB and on the JIRA).
        Why we need this patch is because over in HBASE-11425, Stack had concerns on having BB based compare APIs in the KeyValue.KVComparator.
        In case of BB backed cells we will definitely have cases to handle BB components of the Cell.
        So it is better to move all the comparator things to CellComparator and let them be non static methods. The reason being KVComparator and MetaComparator sort of comparison needs. This patch removes all the comparator code from KV and moves over to CellComparator.
        CellComparator and MetaCellComparator are the new Comparators.
        The patch also contains certain TODOs to show how the BB backed cell related compare methods would be added in the CellComparator methods.
        Also pls note that few compare APIS like compareRows() has been added that takes Cell along with rowOffset and rowLength so that if BB backed Cells are coming we could use the same API and handle them with hasArray condition.
        The patch with BB backed Cell can do further refactoring in certain places where the Cells could be directly passed to the CellComparator so that comparisons could be done on Cells using hasArray() check.
        The getLegacyClassName() is bit of a hack. I think the intention is to remove them but using the String we try to instantiate the respective Comparator Classes. I think that should work even now except that the String that is used for the name would have an odd package name (Interestingly the KeyComparator and MetaKeyComparator was no longer in the code base).

        Show
        ramkrishna.s.vasudevan added a comment - Attaching a rebased patch with some additional changes (addresses some of the review comments posted in RB and on the JIRA). Why we need this patch is because over in HBASE-11425 , Stack had concerns on having BB based compare APIs in the KeyValue.KVComparator. In case of BB backed cells we will definitely have cases to handle BB components of the Cell. So it is better to move all the comparator things to CellComparator and let them be non static methods. The reason being KVComparator and MetaComparator sort of comparison needs. This patch removes all the comparator code from KV and moves over to CellComparator. CellComparator and MetaCellComparator are the new Comparators. The patch also contains certain TODOs to show how the BB backed cell related compare methods would be added in the CellComparator methods. Also pls note that few compare APIS like compareRows() has been added that takes Cell along with rowOffset and rowLength so that if BB backed Cells are coming we could use the same API and handle them with hasArray condition. The patch with BB backed Cell can do further refactoring in certain places where the Cells could be directly passed to the CellComparator so that comparisons could be done on Cells using hasArray() check. The getLegacyClassName() is bit of a hack. I think the intention is to remove them but using the String we try to instantiate the respective Comparator Classes. I think that should work even now except that the String that is used for the name would have an odd package name (Interestingly the KeyComparator and MetaKeyComparator was no longer in the code base).
        Hide
        ramkrishna.s.vasudevan added a comment -

        Pls note that this patch removes some of the APIs in KeyValue.java

        Show
        ramkrishna.s.vasudevan added a comment - Pls note that this patch removes some of the APIs in KeyValue.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12709202/HBASE-10800_3.patch
        against master branch at commit d8b10656d00779e194c3caca118995136babce99.
        ATTACHMENT ID: 12709202

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 146 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 16 warning messages.

        -1 checkstyle. The applied patch generated 1945 checkstyle errors (more than the master's current 1924 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + public int compareRows(Cell left, int loffset, int llength, Cell right, int roffset, int rlength) {
        + public int compareRows(Cell left, int loffset, int llength, byte[] right, int roffset, int rlength) {
        + Bytes.putLong(newKey, rightKey.length - KeyValue.TIMESTAMP_TYPE_SIZE, HConstants.LATEST_TIMESTAMP);
        + && leftKey[KeyValue.ROW_LENGTH_SIZE + diffIdx] == rightKey[KeyValue.ROW_LENGTH_SIZE + diffIdx]) {
        + public int compareRows(Cell left, int loffset, int llength, Cell right, int roffset, int rlength) {
        + public static int findCommonPrefixInQualifierPart(Cell left, Cell right, int qualifierCommonPrefix) {
        + public static int getDelimiter(final byte[] b, int offset, final int length, final int delimiter) {
        + comp = samePrefixComparator.compareCommonRowPrefix(seekCell, currentCell, rowCommonPrefix);
        + public EncodedSeeker createSeeker(CellComparator comparator, HFileBlockDecodingContext decodingCtx) {

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.client.TestScannerTimeout
        org.apache.hadoop.hbase.replication.regionserver.TestReplicationWALReaderManager
        org.apache.hadoop.hbase.wal.TestBoundedRegionGroupingProvider
        org.apache.hadoop.hbase.regionserver.wal.TestWALReplay
        org.apache.hadoop.hbase.regionserver.TestCompoundBloomFilter
        org.apache.hadoop.hbase.wal.TestDefaultWALProviderWithHLogKey
        org.apache.hadoop.hbase.wal.TestDefaultWALProvider
        org.apache.hadoop.hbase.replication.TestReplicationKillMasterRS
        org.apache.hadoop.hbase.replication.regionserver.TestRegionReplicaReplicationEndpoint
        org.apache.hadoop.hbase.regionserver.TestHRegionReplayEvents
        org.apache.hadoop.hbase.regionserver.wal.TestLogRolling
        org.apache.hadoop.hbase.replication.TestReplicationDisableInactivePeer
        org.apache.hadoop.hbase.mapreduce.TestWALRecordReader
        org.apache.hadoop.hbase.replication.regionserver.TestReplicationSink
        org.apache.hadoop.hbase.replication.TestReplicationKillSlaveRS
        org.apache.hadoop.hbase.replication.TestReplicationSource
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsReplication
        org.apache.hadoop.hbase.TestZooKeeper
        org.apache.hadoop.hbase.regionserver.TestRegionReplicaFailover
        org.apache.hadoop.hbase.mapreduce.TestHLogRecordReader
        org.apache.hadoop.hbase.wal.TestWALFactory
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelReplicationWithExpAsString
        org.apache.hadoop.hbase.regionserver.wal.TestDurability
        org.apache.hadoop.hbase.mapreduce.TestWALPlayer
        org.apache.hadoop.hbase.wal.TestWALSplit
        org.apache.hadoop.hbase.replication.TestReplicationSyncUpTool
        org.apache.hadoop.hbase.TestFullLogReconstruction
        org.apache.hadoop.hbase.replication.TestPerTableCFReplication
        org.apache.hadoop.hbase.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.replication.TestReplicationWithTags
        org.apache.hadoop.hbase.regionserver.TestHRegion
        org.apache.hadoop.hbase.regionserver.TestHRegionOnCluster
        org.apache.hadoop.hbase.replication.TestMultiSlaveReplication
        org.apache.hadoop.hbase.replication.TestReplicationEndpoint
        org.apache.hadoop.hbase.replication.TestReplicationKillMasterRSCompressed
        org.apache.hadoop.hbase.replication.TestReplicationSmallTests
        org.apache.hadoop.hbase.replication.TestReplicationChangingPeerRegionservers
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDistributedLogReplay
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService
        org.apache.hadoop.hbase.regionserver.wal.TestProtobufLog
        org.apache.hadoop.hbase.replication.TestMasterReplication
        org.apache.hadoop.hbase.regionserver.TestPerColumnFamilyFlush

        -1 core zombie tests. There are 6 zombie test(s): at org.apache.hadoop.hbase.master.TestRegionPlacement.testRegionPlacement(TestRegionPlacement.java:109)
        at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:380)
        at org.apache.hadoop.hbase.client.TestAdmin1.testTruncateTable(TestAdmin1.java:392)
        at org.apache.hadoop.hbase.client.TestAdmin1.testTruncateTable(TestAdmin1.java:374)
        at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testExportFileSystemState(TestExportSnapshot.java:287)
        at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testConsecutiveExports(TestExportSnapshot.java:211)
        at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testExportFileSystemState(TestExportSnapshot.java:287)
        at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testExportFileSystemState(TestExportSnapshot.java:261)
        at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testSnapshotWithRefsExportFileSystemState(TestExportSnapshot.java:255)
        at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testSnapshotWithRefsExportFileSystemState(TestExportSnapshot.java:239)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12709202/HBASE-10800_3.patch against master branch at commit d8b10656d00779e194c3caca118995136babce99. ATTACHMENT ID: 12709202 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 146 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 16 warning messages. -1 checkstyle . The applied patch generated 1945 checkstyle errors (more than the master's current 1924 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + public int compareRows(Cell left, int loffset, int llength, Cell right, int roffset, int rlength) { + public int compareRows(Cell left, int loffset, int llength, byte[] right, int roffset, int rlength) { + Bytes.putLong(newKey, rightKey.length - KeyValue.TIMESTAMP_TYPE_SIZE, HConstants.LATEST_TIMESTAMP); + && leftKey [KeyValue.ROW_LENGTH_SIZE + diffIdx] == rightKey [KeyValue.ROW_LENGTH_SIZE + diffIdx] ) { + public int compareRows(Cell left, int loffset, int llength, Cell right, int roffset, int rlength) { + public static int findCommonPrefixInQualifierPart(Cell left, Cell right, int qualifierCommonPrefix) { + public static int getDelimiter(final byte[] b, int offset, final int length, final int delimiter) { + comp = samePrefixComparator.compareCommonRowPrefix(seekCell, currentCell, rowCommonPrefix); + public EncodedSeeker createSeeker(CellComparator comparator, HFileBlockDecodingContext decodingCtx) { +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestScannerTimeout org.apache.hadoop.hbase.replication.regionserver.TestReplicationWALReaderManager org.apache.hadoop.hbase.wal.TestBoundedRegionGroupingProvider org.apache.hadoop.hbase.regionserver.wal.TestWALReplay org.apache.hadoop.hbase.regionserver.TestCompoundBloomFilter org.apache.hadoop.hbase.wal.TestDefaultWALProviderWithHLogKey org.apache.hadoop.hbase.wal.TestDefaultWALProvider org.apache.hadoop.hbase.replication.TestReplicationKillMasterRS org.apache.hadoop.hbase.replication.regionserver.TestRegionReplicaReplicationEndpoint org.apache.hadoop.hbase.regionserver.TestHRegionReplayEvents org.apache.hadoop.hbase.regionserver.wal.TestLogRolling org.apache.hadoop.hbase.replication.TestReplicationDisableInactivePeer org.apache.hadoop.hbase.mapreduce.TestWALRecordReader org.apache.hadoop.hbase.replication.regionserver.TestReplicationSink org.apache.hadoop.hbase.replication.TestReplicationKillSlaveRS org.apache.hadoop.hbase.replication.TestReplicationSource org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsReplication org.apache.hadoop.hbase.TestZooKeeper org.apache.hadoop.hbase.regionserver.TestRegionReplicaFailover org.apache.hadoop.hbase.mapreduce.TestHLogRecordReader org.apache.hadoop.hbase.wal.TestWALFactory org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelReplicationWithExpAsString org.apache.hadoop.hbase.regionserver.wal.TestDurability org.apache.hadoop.hbase.mapreduce.TestWALPlayer org.apache.hadoop.hbase.wal.TestWALSplit org.apache.hadoop.hbase.replication.TestReplicationSyncUpTool org.apache.hadoop.hbase.TestFullLogReconstruction org.apache.hadoop.hbase.replication.TestPerTableCFReplication org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.replication.TestReplicationWithTags org.apache.hadoop.hbase.regionserver.TestHRegion org.apache.hadoop.hbase.regionserver.TestHRegionOnCluster org.apache.hadoop.hbase.replication.TestMultiSlaveReplication org.apache.hadoop.hbase.replication.TestReplicationEndpoint org.apache.hadoop.hbase.replication.TestReplicationKillMasterRSCompressed org.apache.hadoop.hbase.replication.TestReplicationSmallTests org.apache.hadoop.hbase.replication.TestReplicationChangingPeerRegionservers org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDistributedLogReplay org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService org.apache.hadoop.hbase.regionserver.wal.TestProtobufLog org.apache.hadoop.hbase.replication.TestMasterReplication org.apache.hadoop.hbase.regionserver.TestPerColumnFamilyFlush -1 core zombie tests . There are 6 zombie test(s): at org.apache.hadoop.hbase.master.TestRegionPlacement.testRegionPlacement(TestRegionPlacement.java:109) at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:380) at org.apache.hadoop.hbase.client.TestAdmin1.testTruncateTable(TestAdmin1.java:392) at org.apache.hadoop.hbase.client.TestAdmin1.testTruncateTable(TestAdmin1.java:374) at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testExportFileSystemState(TestExportSnapshot.java:287) at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testConsecutiveExports(TestExportSnapshot.java:211) at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testExportFileSystemState(TestExportSnapshot.java:287) at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testExportFileSystemState(TestExportSnapshot.java:261) at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testSnapshotWithRefsExportFileSystemState(TestExportSnapshot.java:255) at org.apache.hadoop.hbase.snapshot.TestExportSnapshot.testSnapshotWithRefsExportFileSystemState(TestExportSnapshot.java:239) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13551//console This message is automatically generated.
        Hide
        ramkrishna vasudevan added a comment -

        Oops. Wil checkout the test failures

        Show
        ramkrishna vasudevan added a comment - Oops. Wil checkout the test failures
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch that addresses the test case issues. Wraps the long line and has some checkStyle comment fixes also.

        Show
        ramkrishna.s.vasudevan added a comment - Patch that addresses the test case issues. Wraps the long line and has some checkStyle comment fixes also.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723306/HBASE-10800_4.patch
        against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d.
        ATTACHMENT ID: 12723306

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 150 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13573//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723306/HBASE-10800_4.patch against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d. ATTACHMENT ID: 12723306 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 150 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13573//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Latest patch that solves a compilation issue.

        Show
        ramkrishna.s.vasudevan added a comment - Latest patch that solves a compilation issue.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723308/HBASE-10800_4.patch
        against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d.
        ATTACHMENT ID: 12723308

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 150 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13575//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723308/HBASE-10800_4.patch against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d. ATTACHMENT ID: 12723308 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 150 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13575//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch against trunk. The patch was not applying because of the recent purge of Writers/Readers.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch against trunk. The patch was not applying because of the recent purge of Writers/Readers.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723314/HBASE-10800_5.patch
        against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d.
        ATTACHMENT ID: 12723314

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 150 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 4 warning messages.

        -1 checkstyle. The applied patch generated 1933 checkstyle errors (more than the master's current 1921 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.regionserver.TestCompoundBloomFilter

        -1 core zombie tests. There are 4 zombie test(s): at org.apache.hadoop.hbase.regionserver.TestEndToEndSplitTransaction.testFromClientSideWhileSplitting(TestEndToEndSplitTransaction.java:202)
        at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:380)
        at org.apache.hadoop.hbase.master.TestRegionPlacement.testRegionPlacement(TestRegionPlacement.java:109)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723314/HBASE-10800_5.patch against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d. ATTACHMENT ID: 12723314 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 150 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 4 warning messages. -1 checkstyle . The applied patch generated 1933 checkstyle errors (more than the master's current 1921 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestCompoundBloomFilter -1 core zombie tests . There are 4 zombie test(s): at org.apache.hadoop.hbase.regionserver.TestEndToEndSplitTransaction.testFromClientSideWhileSplitting(TestEndToEndSplitTransaction.java:202) at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:380) at org.apache.hadoop.hbase.master.TestRegionPlacement.testRegionPlacement(TestRegionPlacement.java:109) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13576//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        New patch. Addresses the failed test case TestCompoundBloomFilter.

        Show
        ramkrishna.s.vasudevan added a comment - New patch. Addresses the failed test case TestCompoundBloomFilter.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723386/HBASE-10800_6.patch
        against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d.
        ATTACHMENT ID: 12723386

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 150 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 3 warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723386/HBASE-10800_6.patch against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d. ATTACHMENT ID: 12723386 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 150 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 3 warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13582//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Any chance of reviews here? Would be useful for HBASE-11425 review comments also.

        Show
        ramkrishna.s.vasudevan added a comment - Any chance of reviews here? Would be useful for HBASE-11425 review comments also.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch. This patch addresses the review comments in RB and this patch is much bigger than the previous ones because of the movement of the public static COMPARATOR variables in KV to CellComparator.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch. This patch addresses the review comments in RB and this patch is much bigger than the previous ones because of the movement of the public static COMPARATOR variables in KV to CellComparator.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723845/HBASE-10800_7.patch
        against master branch at commit b6756b39c2ff5675f96a6e82dc4d68ec437f01c4.
        ATTACHMENT ID: 12723845

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 243 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 4 warning messages.

        -1 checkstyle. The applied patch generated 1935 checkstyle errors (more than the master's current 1921 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + public static Cell getMidpoint(final CellComparator comparator, final Cell left, final Cell right) {
        + public static int getDelimiter(final byte[] b, int offset, final int length, final int delimiter)

        { + }

        else if (comparatorClassName.equals(CellComparator.RAW_COMPARATOR.getLegacyKeyComparatorName())) {

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723845/HBASE-10800_7.patch against master branch at commit b6756b39c2ff5675f96a6e82dc4d68ec437f01c4. ATTACHMENT ID: 12723845 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 243 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 4 warning messages. -1 checkstyle . The applied patch generated 1935 checkstyle errors (more than the master's current 1921 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + public static Cell getMidpoint(final CellComparator comparator, final Cell left, final Cell right) { + public static int getDelimiter(final byte[] b, int offset, final int length, final int delimiter) { + } else if (comparatorClassName.equals(CellComparator.RAW_COMPARATOR.getLegacyKeyComparatorName())) { +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13616//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        I have another updated patch here which tries to remove all API that accept two byte[]. Running test suite. If all passes that can be taken as the final one. But we have some additional objects created in certain cases but I think that would be needed. Will upload shortly.

        Show
        ramkrishna.s.vasudevan added a comment - I have another updated patch here which tries to remove all API that accept two byte[]. Running test suite. If all passes that can be taken as the final one. But we have some additional objects created in certain cases but I think that would be needed. Will upload shortly.
        Hide
        ramkrishna.s.vasudevan added a comment -

        This patch is much bigger patch but tries to avoid two byte[] compare methods in CellComparator. Only one API is left out compareRows. This needs some changes in the HRegion and the filter APIs. I think we can take that up later as another subtask because it involves filter API changes. ( or could even do with HBASE-11425).
        Pls take note of the StoreFile.java and Bytes.java changes. Those are some critical places where we handle the compare APIs for bloom APIs.

        Show
        ramkrishna.s.vasudevan added a comment - This patch is much bigger patch but tries to avoid two byte[] compare methods in CellComparator. Only one API is left out compareRows. This needs some changes in the HRegion and the filter APIs. I think we can take that up later as another subtask because it involves filter API changes. ( or could even do with HBASE-11425 ). Pls take note of the StoreFile.java and Bytes.java changes. Those are some critical places where we handle the compare APIs for bloom APIs.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724162/HBASE-10800_11.patch
        against master branch at commit 80dbf06651e527ec0421ce51e4712ffb2f1d078b.
        ATTACHMENT ID: 12724162

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 251 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 10 warning messages.

        -1 checkstyle. The applied patch generated 1932 checkstyle errors (more than the master's current 1921 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + final int rfamilyOffset, final int rfamilylength, final int rqualOffset, final int rqualLength) {
        + // CLEANUP : in order to do this we may have to modify some code HRegion.next() and will involve a
        + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0,
        + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0,
        + res = (-(generalBloomFilterWriter.getComparator().compare(lastBloomKeyOnlyKV, bloomKey,
        + + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "]";
        + + Bytes.toString(right) + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "]";

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724162/HBASE-10800_11.patch against master branch at commit 80dbf06651e527ec0421ce51e4712ffb2f1d078b. ATTACHMENT ID: 12724162 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 251 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 10 warning messages. -1 checkstyle . The applied patch generated 1932 checkstyle errors (more than the master's current 1921 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + final int rfamilyOffset, final int rfamilylength, final int rqualOffset, final int rqualLength) { + // CLEANUP : in order to do this we may have to modify some code HRegion.next() and will involve a + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0, + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0, + res = (-(generalBloomFilterWriter.getComparator().compare(lastBloomKeyOnlyKV, bloomKey, + + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "] "; + + Bytes.toString(right) + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "] "; +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13640//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch that wraps the long lines.

        Show
        ramkrishna.s.vasudevan added a comment - Patch that wraps the long lines.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724186/HBASE-10800_12.patch
        against master branch at commit 80dbf06651e527ec0421ce51e4712ffb2f1d078b.
        ATTACHMENT ID: 12724186

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 251 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 10 warning messages.

        -1 checkstyle. The applied patch generated 1926 checkstyle errors (more than the master's current 1921 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724186/HBASE-10800_12.patch against master branch at commit 80dbf06651e527ec0421ce51e4712ffb2f1d078b. ATTACHMENT ID: 12724186 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 251 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 10 warning messages. -1 checkstyle . The applied patch generated 1926 checkstyle errors (more than the master's current 1921 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13645//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch based on review comments.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch based on review comments.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724751/HBASE-10800_14.patch
        against master branch at commit e75c6201c69e57416525135a397a971ad4d1b902.
        ATTACHMENT ID: 12724751

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 251 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 16 warning messages.

        -1 checkstyle. The applied patch generated 1934 checkstyle errors (more than the master's current 1912 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + final int index = Bytes.searchDelimiterIndex(comparator, 0, comparator.length, ParseConstants.COLON);
        + final int rfamilyOffset, final int rfamilylength, final int rqualOffset, final int rqualLength) {
        + // TODO : CLEANUP : in order to do this we may have to modify some code HRegion.next() and will involve a
        + * @param coff the offset of the column hint if provided, if not offset of the currentCell's qualifier
        + * @param clen the length of the column hint if provided, if not length of the currentCell's qualifier
        + return Bytes.findCommonPrefix(left.getFamilyArray(), right.getFamilyArray(), left.getFamilyLength()
        + return matchingRow(a, b) && matchingFamily(a, b) && matchingQualifier(a, b) && matchingTimestamp(a, b)
        + public static int searchDelimiterIndexInReverse(final byte[] b, final int offset, final int length,
        + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0,
        + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0,

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.regionserver.TestBlocksRead

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724751/HBASE-10800_14.patch against master branch at commit e75c6201c69e57416525135a397a971ad4d1b902. ATTACHMENT ID: 12724751 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 251 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 16 warning messages. -1 checkstyle . The applied patch generated 1934 checkstyle errors (more than the master's current 1912 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + final int index = Bytes.searchDelimiterIndex(comparator, 0, comparator.length, ParseConstants.COLON); + final int rfamilyOffset, final int rfamilylength, final int rqualOffset, final int rqualLength) { + // TODO : CLEANUP : in order to do this we may have to modify some code HRegion.next() and will involve a + * @param coff the offset of the column hint if provided, if not offset of the currentCell's qualifier + * @param clen the length of the column hint if provided, if not length of the currentCell's qualifier + return Bytes.findCommonPrefix(left.getFamilyArray(), right.getFamilyArray(), left.getFamilyLength() + return matchingRow(a, b) && matchingFamily(a, b) && matchingQualifier(a, b) && matchingTimestamp(a, b) + public static int searchDelimiterIndexInReverse(final byte[] b, final int offset, final int length, + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0, + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0, +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestBlocksRead Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13670//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Test case correction.

        Show
        ramkrishna.s.vasudevan added a comment - Test case correction.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724768/HBASE-10800_15.patch
        against master branch at commit e75c6201c69e57416525135a397a971ad4d1b902.
        ATTACHMENT ID: 12724768

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 251 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 14 warning messages.

        -1 checkstyle. The applied patch generated 1927 checkstyle errors (more than the master's current 1912 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + public static int findCommonPrefixInQualifierPart(Cell left, Cell right, int qualifierCommonPrefix) {
        + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0,
        + if (-(getComparator().compare(splitCell, bb.array(), bb.arrayOffset(), bb.limit())) >= 0) {
        + + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "]";
        + + Bytes.toString(right) + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "]";

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724768/HBASE-10800_15.patch against master branch at commit e75c6201c69e57416525135a397a971ad4d1b902. ATTACHMENT ID: 12724768 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 251 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 14 warning messages. -1 checkstyle . The applied patch generated 1927 checkstyle errors (more than the master's current 1912 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + public static int findCommonPrefixInQualifierPart(Cell left, Cell right, int qualifierCommonPrefix) { + Bytes.equals(newKey.getRowArray(), newKey.getRowOffset(), newKey.getRowLength(), expectedArray, 0, + if (-(getComparator().compare(splitCell, bb.array(), bb.arrayOffset(), bb.limit())) >= 0) { + + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "] "; + + Bytes.toString(right) + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) + "] "; +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13673//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Wraps long lines. Fixes checkstyle comments related to Unused Imports.
        Fixes javadoc warnings.
        CellComparator.compareKeyBasedOnColHint() I have added back the familyOffset and familyLength here because for cases where we get the compareKeyForNextRow the family offset and length should be 0 which we cannot get from the cell. Hence it is better we explicitly pass them there.

        Show
        ramkrishna.s.vasudevan added a comment - Wraps long lines. Fixes checkstyle comments related to Unused Imports. Fixes javadoc warnings. CellComparator.compareKeyBasedOnColHint() I have added back the familyOffset and familyLength here because for cases where we get the compareKeyForNextRow the family offset and length should be 0 which we cannot get from the cell. Hence it is better we explicitly pass them there.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724868/HBASE-10800_16.patch
        against master branch at commit e75c6201c69e57416525135a397a971ad4d1b902.
        ATTACHMENT ID: 12724868

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 251 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 4 warning messages.

        -1 checkstyle. The applied patch generated 1914 checkstyle errors (more than the master's current 1912 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724868/HBASE-10800_16.patch against master branch at commit e75c6201c69e57416525135a397a971ad4d1b902. ATTACHMENT ID: 12724868 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 251 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 4 warning messages. -1 checkstyle . The applied patch generated 1914 checkstyle errors (more than the master's current 1912 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13678//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Is it fine to commit this? I need to add a fat release note on this. Stack and Anoop Sam John? Can continue some follow on work based on this. Anoop's ServerCell patch once integrated would help us to proceed with other sub tasks for HBASe-11425. With that majority of review comments in HBASE-11425's RB would be addressed.

        Show
        ramkrishna.s.vasudevan added a comment - Is it fine to commit this? I need to add a fat release note on this. Stack and Anoop Sam John ? Can continue some follow on work based on this. Anoop's ServerCell patch once integrated would help us to proceed with other sub tasks for HBASe-11425. With that majority of review comments in HBASE-11425 's RB would be addressed.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Except for the comment related to

        public final int compare(Cell left, byte[] key, int offset, int length)
        

        all other changes are done. As said in the RB this API is extensively used in blooms, index and 2 places in HalfStoreFileReader. The other alternative is to change every where to KeyOnlyKVs but that would create more short living objects particularly in the bloom area.

        Show
        ramkrishna.s.vasudevan added a comment - Except for the comment related to public final int compare(Cell left, byte [] key, int offset, int length) all other changes are done. As said in the RB this API is extensively used in blooms, index and 2 places in HalfStoreFileReader. The other alternative is to change every where to KeyOnlyKVs but that would create more short living objects particularly in the bloom area.
        Hide
        Anoop Sam John added a comment -

        +1

        compare(Cell left, byte[] key, int offset, int length)

        Ya we can do this in a follow up jira.

        Show
        Anoop Sam John added a comment - +1 compare(Cell left, byte[] key, int offset, int length) Ya we can do this in a follow up jira.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12725781/HBASE-10800_18.patch
        against master branch at commit 682a29a57f73b836859b3d3e1048fc82d64e8fe3.
        ATTACHMENT ID: 12725781

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 251 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//artifact/patchprocess/checkstyle-aggregate.html

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725781/HBASE-10800_18.patch against master branch at commit 682a29a57f73b836859b3d3e1048fc82d64e8fe3. ATTACHMENT ID: 12725781 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 251 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13721//console This message is automatically generated.
        Hide
        stack added a comment -

        Sorry. Slow on review. I have one running now. Let me finish first ramkrishna.s.vasudevan

        Show
        stack added a comment - Sorry. Slow on review. I have one running now. Let me finish first ramkrishna.s.vasudevan
        Hide
        stack added a comment -

        I finally got through the seven pages of patch. There are some concerns posted on RB. Patch is too big. Comments are too many up on rb. Will be hard to ensure all addressed in subsequent versions of the patch.

        Here is a overarching comment from rb:

        This is a fundamental change... Previous, the comparator knew about the serialization and exploited this fact avoiding re-parse of KV part lengths and offsets. In this patch we are moving the comparator to go via the Cell Interface only it does not expose speedup methods, those methods that allow you pass in a length or offset already calculated. While this patch preserves these 'carry-over' methods, they'll have to be removed eventually... the carry-over methods just won't work when we have a different Cell type. So, we will see slowdown comparing.... unless the Cell internally does what these carry-over methods are doing and keeps them cached internally (HBASE-13448)

        Show
        stack added a comment - I finally got through the seven pages of patch. There are some concerns posted on RB. Patch is too big. Comments are too many up on rb. Will be hard to ensure all addressed in subsequent versions of the patch. Here is a overarching comment from rb: This is a fundamental change... Previous, the comparator knew about the serialization and exploited this fact avoiding re-parse of KV part lengths and offsets. In this patch we are moving the comparator to go via the Cell Interface only it does not expose speedup methods, those methods that allow you pass in a length or offset already calculated. While this patch preserves these 'carry-over' methods, they'll have to be removed eventually... the carry-over methods just won't work when we have a different Cell type. So, we will see slowdown comparing.... unless the Cell internally does what these carry-over methods are doing and keeps them cached internally ( HBASE-13448 )
        Hide
        ramkrishna.s.vasudevan added a comment -

        Trying for QA run.

        Show
        ramkrishna.s.vasudevan added a comment - Trying for QA run.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Retrying

        Show
        ramkrishna.s.vasudevan added a comment - Retrying
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12728316/HBASE-10800_23.patch
        against master branch at commit 4182fc1a9bc261f50efd7efd27c61a702bc1bfbf.
        ATTACHMENT ID: 12728316

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 258 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 15 warning messages.

        -1 checkstyle. The applied patch generated 1904 checkstyle errors (more than the master's current 1898 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.util.TestHBaseFsck

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12728316/HBASE-10800_23.patch against master branch at commit 4182fc1a9bc261f50efd7efd27c61a702bc1bfbf. ATTACHMENT ID: 12728316 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 258 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 15 warning messages. -1 checkstyle . The applied patch generated 1904 checkstyle errors (more than the master's current 1898 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.util.TestHBaseFsck Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13828//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated the javadoc and checkstyle comments.

        Show
        ramkrishna.s.vasudevan added a comment - Updated the javadoc and checkstyle comments.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12728338/HBASE-10800_23.patch
        against master branch at commit f2e1238f9812ebe0257ad4b3af1200230231529a.
        ATTACHMENT ID: 12728338

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 258 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 15 warning messages.

        -1 checkstyle. The applied patch generated 1904 checkstyle errors (more than the master's current 1898 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.util.TestHBaseFsck

        -1 core zombie tests. There are 5 zombie test(s): at org.apache.hadoop.hbase.mapreduce.TestTimeRangeMapRed.testTimeRangeMapRed(TestTimeRangeMapRed.java:163)
        at org.apache.hadoop.hbase.mapreduce.TestCellCounter.testCellCounteEndTimeRange(TestCellCounter.java:176)
        at org.apache.hadoop.hbase.mapreduce.TestImportTsv.testMROnTable(TestImportTsv.java:116)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12728338/HBASE-10800_23.patch against master branch at commit f2e1238f9812ebe0257ad4b3af1200230231529a. ATTACHMENT ID: 12728338 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 258 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 15 warning messages. -1 checkstyle . The applied patch generated 1904 checkstyle errors (more than the master's current 1898 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.util.TestHBaseFsck -1 core zombie tests . There are 5 zombie test(s): at org.apache.hadoop.hbase.mapreduce.TestTimeRangeMapRed.testTimeRangeMapRed(TestTimeRangeMapRed.java:163) at org.apache.hadoop.hbase.mapreduce.TestCellCounter.testCellCounteEndTimeRange(TestCellCounter.java:176) at org.apache.hadoop.hbase.mapreduce.TestImportTsv.testMROnTable(TestImportTsv.java:116) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13830//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12728368/HBASE-10800_24.patch
        against master branch at commit f2e1238f9812ebe0257ad4b3af1200230231529a.
        ATTACHMENT ID: 12728368

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 258 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.util.TestHBaseFsck

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//artifact/patchprocess/checkstyle-aggregate.html

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12728368/HBASE-10800_24.patch against master branch at commit f2e1238f9812ebe0257ad4b3af1200230231529a. ATTACHMENT ID: 12728368 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 258 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.util.TestHBaseFsck Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13834//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        The test failure seems to be occuring always with and without patch. So seems like it is unrelated.

        Show
        ramkrishna.s.vasudevan added a comment - The test failure seems to be occuring always with and without patch. So seems like it is unrelated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch addressing latest RB comments. Trying QA .

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch addressing latest RB comments. Trying QA .
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12729429/HBASE-10800_25.patch
        against master branch at commit 558cac91d05aef811a61b9bcd86078a0cb13c879.
        ATTACHMENT ID: 12729429

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 262 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 3 warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12729429/HBASE-10800_25.patch against master branch at commit 558cac91d05aef811a61b9bcd86078a0cb13c879. ATTACHMENT ID: 12729429 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 262 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 3 warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13896//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Upated the javadoc comment that was coming from CellComparator.

        Show
        ramkrishna.s.vasudevan added a comment - Upated the javadoc comment that was coming from CellComparator.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12729469/HBASE-10800_26.patch
        against master branch at commit 3a9c2b0c5510c499371e9f387fa85c5fdeb76628.
        ATTACHMENT ID: 12729469

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 262 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13898//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12729469/HBASE-10800_26.patch against master branch at commit 3a9c2b0c5510c499371e9f387fa85c5fdeb76628. ATTACHMENT ID: 12729469 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 262 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13898//console This message is automatically generated.
        Hide
        Anoop Sam John added a comment -

        FFT.java

        private static Class<? extends CellComparator> getComparatorClass(
              String comparatorClassName) throws IOException {
            try {
              // HFile V2 legacy comparator class names.
              if (comparatorClassName.equals(CellComparator.COMPARATOR.getLegacyKeyComparatorName())) {
                comparatorClassName = CellComparator.COMPARATOR.getClass().getName();
              } else if (comparatorClassName.equals(CellComparator.META_COMPARATOR
                  .getLegacyKeyComparatorName())) {
                comparatorClassName = CellComparator.META_COMPARATOR.getClass().getName();
              }
        
              // if the name wasn't one of the legacy names, maybe its a legit new kind of comparator.
              // TODO : This is deprecated. While removing this code introduce
              // Bytes.getV2LegacyKeyComparatorName()
              if (comparatorClassName.equals(KeyValue.RAW_COMPARATOR.getClass().getName())) {
                // Return null for Bytes.BYTES_RAWCOMPARATOR
                return null;
              } else if (comparatorClassName.equals(KeyValue.RAW_COMPARATOR.getLegacyKeyComparatorName())) {
                return null;
              } else {
                return (Class<? extends CellComparator>) Class.forName(comparatorClassName);
              }
            } catch (ClassNotFoundException ex) {
              throw new IOException(ex);
            }
          }
        

        CellComparator.getLegacyKeyComparatorName() return same value as KVComparator.getLegacyKeyComparatorName(). When the className in FFT for a file is, KVComparator, then also we need to return CellComparator instance.
        This check also required for completeness.

        Functionally it wont create any BC issue as we still were writing the legacy class name only to new files also.

        Show
        Anoop Sam John added a comment - FFT.java private static Class <? extends CellComparator> getComparatorClass( String comparatorClassName) throws IOException { try { // HFile V2 legacy comparator class names. if (comparatorClassName.equals(CellComparator.COMPARATOR.getLegacyKeyComparatorName())) { comparatorClassName = CellComparator.COMPARATOR.getClass().getName(); } else if (comparatorClassName.equals(CellComparator.META_COMPARATOR .getLegacyKeyComparatorName())) { comparatorClassName = CellComparator.META_COMPARATOR.getClass().getName(); } // if the name wasn't one of the legacy names, maybe its a legit new kind of comparator. // TODO : This is deprecated. While removing this code introduce // Bytes.getV2LegacyKeyComparatorName() if (comparatorClassName.equals(KeyValue.RAW_COMPARATOR.getClass().getName())) { // Return null for Bytes.BYTES_RAWCOMPARATOR return null ; } else if (comparatorClassName.equals(KeyValue.RAW_COMPARATOR.getLegacyKeyComparatorName())) { return null ; } else { return ( Class <? extends CellComparator>) Class .forName(comparatorClassName); } } catch (ClassNotFoundException ex) { throw new IOException(ex); } } CellComparator.getLegacyKeyComparatorName() return same value as KVComparator.getLegacyKeyComparatorName(). When the className in FFT for a file is, KVComparator, then also we need to return CellComparator instance. This check also required for completeness. Functionally it wont create any BC issue as we still were writing the legacy class name only to new files also.
        Hide
        Anoop Sam John added a comment -

        Stack's comment from RB

        Only real concern is some exposure of byte arrays but we can fix that in subsequent patches. There is also a question of how often we will be creating those key only KVs...

        The APIs exposing byte[] (still) will go away with other subtasks.
        I am checking the creation of KeyOnlyKeyValues for the compare. Some places we can avoid object creation every time (In read hot path).. Just adding TODOs and once this is in can do as sub tasks..

        Show
        Anoop Sam John added a comment - Stack's comment from RB Only real concern is some exposure of byte arrays but we can fix that in subsequent patches. There is also a question of how often we will be creating those key only KVs... The APIs exposing byte[] (still) will go away with other subtasks. I am checking the creation of KeyOnlyKeyValues for the compare. Some places we can avoid object creation every time (In read hot path).. Just adding TODOs and once this is in can do as sub tasks..
        Hide
        stack added a comment -

        TODOs and once this is in can do as sub tasks..

        Sure. Just file as blockers before this is committed.

        Is there a new patch to look at? Just page #1 is the important one I think.

        Show
        stack added a comment - TODOs and once this is in can do as sub tasks.. Sure. Just file as blockers before this is committed. Is there a new patch to look at? Just page #1 is the important one I think.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Hi all,
        I was on vacation over the weekend. I will look check and come back with a latest patch for review so that we can commit this early next week.

        Show
        ramkrishna.s.vasudevan added a comment - Hi all, I was on vacation over the weekend. I will look check and come back with a latest patch for review so that we can commit this early next week.
        Hide
        Anoop Sam John added a comment -

        HalfStoreFileReader
        ByteBuffer k = this.delegate.getKey();

        • return this.delegate.getReader().getComparator().
        • compareFlatKey(k.array(), k.arrayOffset(), k.limit(),
        • splitkey, 0, splitkey.length) < 0;
          + return (this.delegate.getReader().getComparator().
          + compare(splitCell, k.array(), k.arrayOffset(), k.limit()
          + )) > 0;
          Now the order of params changed, so the condition to be >=0
        Show
        Anoop Sam John added a comment - HalfStoreFileReader ByteBuffer k = this.delegate.getKey(); return this.delegate.getReader().getComparator(). compareFlatKey(k.array(), k.arrayOffset(), k.limit(), splitkey, 0, splitkey.length) < 0; + return (this.delegate.getReader().getComparator(). + compare(splitCell, k.array(), k.arrayOffset(), k.limit() + )) > 0; Now the order of params changed, so the condition to be >=0
        Hide
        Anoop Sam John added a comment -

        My bad.. Just leave the above comment... Got confused at first.. After more careful check and discuss with Ram, seems change is just fine

        Show
        Anoop Sam John added a comment - My bad.. Just leave the above comment... Got confused at first.. After more careful check and discuss with Ram, seems change is just fine
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch. Thanks to Anoop for fixing a couple of RB comments in ths patch.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch. Thanks to Anoop for fixing a couple of RB comments in ths patch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12730150/HBASE-10800_27.patch
        against master branch at commit 1f02fffd044245fe777c2194517234950d530b35.
        ATTACHMENT ID: 12730150

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 262 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//artifact/patchprocess/checkstyle-aggregate.html

        Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12730150/HBASE-10800_27.patch against master branch at commit 1f02fffd044245fe777c2194517234950d530b35. ATTACHMENT ID: 12730150 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 262 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 2 warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13934//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Removes the 2 javadoc warnings.

        Show
        ramkrishna.s.vasudevan added a comment - Removes the 2 javadoc warnings.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Stack
        Any chance of a final review here and a +1?

        Show
        ramkrishna.s.vasudevan added a comment - Stack Any chance of a final review here and a +1?
        Hide
        stack added a comment -

        +1 on last patch. Up on RB I put some notes on TODOs you should add on commit. There is a question too but I think I see the answer on page #6 so it should not block commit. If the answer is a problem, lets do in a follow-on issue.

        Show
        stack added a comment - +1 on last patch. Up on RB I put some notes on TODOs you should add on commit. There is a question too but I think I see the answer on page #6 so it should not block commit. If the answer is a problem, lets do in a follow-on issue.
        Hide
        stack added a comment -

        ramkrishna.s.vasudevan See +1 in comment above.

        Show
        stack added a comment - ramkrishna.s.vasudevan See +1 in comment above.
        Hide
        ramkrishna.s.vasudevan added a comment -

        There is a question too but I think I see the answer on page #6 so it should not block commit. If the answer is a problem, lets do in a follow-on issue.

        you are right. We have already moved this to HfileWriterImpl and hence the tests also have been moved. Because it would look ugly if we call Writer from the TestKeyValue or TestCellComparator.

        Stack and Anoop Sam John thanks for the reviews and comments. It should have been a tough task.

        Show
        ramkrishna.s.vasudevan added a comment - There is a question too but I think I see the answer on page #6 so it should not block commit. If the answer is a problem, lets do in a follow-on issue. you are right. We have already moved this to HfileWriterImpl and hence the tests also have been moved. Because it would look ugly if we call Writer from the TestKeyValue or TestCellComparator. Stack and Anoop Sam John thanks for the reviews and comments. It should have been a tough task.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Pushed to master. Will work on follow on JIRAs to fill up the TODOs.

        Show
        ramkrishna.s.vasudevan added a comment - Pushed to master. Will work on follow on JIRAs to fill up the TODOs.
        Hide
        ramkrishna.s.vasudevan added a comment - - edited

        Patch that was pushed. Should we add a release note now or when we deprecate the KVComparator? I will file a blocker against 2.0 to remove KVComparator.

        {Edit}

        Should I file a blocker against 3.0 to remove KvComparator? or is it too early to do so?

        Show
        ramkrishna.s.vasudevan added a comment - - edited Patch that was pushed. Should we add a release note now or when we deprecate the KVComparator? I will file a blocker against 2.0 to remove KVComparator. {Edit} Should I file a blocker against 3.0 to remove KvComparator? or is it too early to do so?
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #6456 (See https://builds.apache.org/job/HBase-TRUNK/6456/)
        HBASE-10800 - Use CellComparator instead of KVComparator (Ram) (ramkrishna: rev 977f867439e960c668ee6311e47c904efc40f219)

        • hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueTestUtil.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterImpl.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat2.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFileManager.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompoundBloomFilterBase.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreEngine.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestReversibleScanners.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/HFilePerformanceEvaluation.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/codec/KeyValueCodecWithTags.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestReseekTo.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
        • hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/decode/PrefixTreeArrayScanner.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        • hbase-client/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
        • hbase-common/src/test/java/org/apache/hadoop/hbase/TestCellComparator.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/CollectionBackedScanner.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileSeek.java
        • hbase-common/src/test/java/org/apache/hadoop/hbase/codec/TestKeyValueCodecWithTags.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreEngine.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MockStoreFile.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Region.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/util/Bytes.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/KeyValueSerialization.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRecoveredEdits.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeMultiFileWriter.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFixture.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedStoreScanner.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueHeap.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java
        • hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/PrefixTreeCodec.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
        • hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestOperation.java
        • hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/decode/PrefixTreeCell.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultStoreEngine.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestDependentColumnFilter.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestPrefixTreeEncoding.java
        • hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestBytes.java
        • hbase-it/src/test/java/org/apache/hadoop/hbase/mapreduce/IntegrationTestImportTsv.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationWALEntryFilters.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFileManager.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedKeyValueHeap.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompoundBloomFilter.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestSeekToBlockWithEncoders.java
        • hbase-common/src/test/java/org/apache/hadoop/hbase/codec/TestCellCodec.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/TextSortReducer.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/KeyValueSortReducer.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestSingleColumnValueExcludeFilter.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/MetaMockingUtil.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStripeStoreFileManager.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ParseFilter.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueScanFixture.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/SimpleTotalOrderPartitioner.java
        • hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilter.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterFactory.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfo.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/codec/TestCellMessageCodec.java
        • hbase-client/src/main/java/org/apache/hadoop/hbase/client/Result.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilterFactory.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStripeStoreEngine.java
        • hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/TestPrefixTreeSearcher.java
        • hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestClientNoCluster.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/GetClosestRowBeforeTracker.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/codec/KeyValueCodec.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileWriterV2.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
        • hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/data/TestRowDataNumberStrings.java
        • hbase-common/src/test/java/org/apache/hadoop/hbase/codec/TestCellCodecWithTags.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCellSkipListSet.java
        • hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/PrefixTreeSeeker.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestBufferedDataBlockEncoder.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestFixedFileTrailer.java
        • hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetaCache.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompoundBloomFilterWriter.java
        • hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/data/TestRowDataSearcherRowMiss.java
        • hbase-examples/src/test/java/org/apache/hadoop/hbase/types/TestPBCell.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/ByteBloomFilter.java
        • hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/BaseTestRowData.java
        • hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/data/TestRowDataSimple.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileWriterV3.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestSeekTo.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStripeCompactor.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/util/test/RedundantKVGenerator.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellSkipListSet.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestResult.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java
        • hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        • hbase-common/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #6456 (See https://builds.apache.org/job/HBase-TRUNK/6456/ ) HBASE-10800 - Use CellComparator instead of KVComparator (Ram) (ramkrishna: rev 977f867439e960c668ee6311e47c904efc40f219) hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueTestUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterImpl.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat2.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFileManager.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompoundBloomFilterBase.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreEngine.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestReversibleScanners.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java hbase-server/src/test/java/org/apache/hadoop/hbase/HFilePerformanceEvaluation.java hbase-common/src/main/java/org/apache/hadoop/hbase/codec/KeyValueCodecWithTags.java hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestReseekTo.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/decode/PrefixTreeArrayScanner.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java hbase-client/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java hbase-common/src/test/java/org/apache/hadoop/hbase/TestCellComparator.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/CollectionBackedScanner.java hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileSeek.java hbase-common/src/test/java/org/apache/hadoop/hbase/codec/TestKeyValueCodecWithTags.java hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreEngine.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MockStoreFile.java hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Region.java hbase-common/src/main/java/org/apache/hadoop/hbase/util/Bytes.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/KeyValueSerialization.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRecoveredEdits.java hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeMultiFileWriter.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFixture.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedStoreScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueHeap.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/PrefixTreeCodec.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestOperation.java hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/decode/PrefixTreeCell.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultStoreEngine.java hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestDependentColumnFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestPrefixTreeEncoding.java hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestBytes.java hbase-it/src/test/java/org/apache/hadoop/hbase/mapreduce/IntegrationTestImportTsv.java hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationWALEntryFilters.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFileManager.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedKeyValueHeap.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompoundBloomFilter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestSeekToBlockWithEncoders.java hbase-common/src/test/java/org/apache/hadoop/hbase/codec/TestCellCodec.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/TextSortReducer.java hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/KeyValueSortReducer.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestSingleColumnValueExcludeFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/MetaMockingUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStripeStoreFileManager.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ParseFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueScanFixture.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/SimpleTotalOrderPartitioner.java hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilter.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterFactory.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfo.java hbase-server/src/test/java/org/apache/hadoop/hbase/codec/TestCellMessageCodec.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/Result.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilterFactory.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStripeStoreEngine.java hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/TestPrefixTreeSearcher.java hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestClientNoCluster.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/GetClosestRowBeforeTracker.java hbase-common/src/main/java/org/apache/hadoop/hbase/codec/KeyValueCodec.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileWriterV2.java hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/data/TestRowDataNumberStrings.java hbase-common/src/test/java/org/apache/hadoop/hbase/codec/TestCellCodecWithTags.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCellSkipListSet.java hbase-prefix-tree/src/main/java/org/apache/hadoop/hbase/codec/prefixtree/PrefixTreeSeeker.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestBufferedDataBlockEncoder.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestFixedFileTrailer.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetaCache.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompoundBloomFilterWriter.java hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/data/TestRowDataSearcherRowMiss.java hbase-examples/src/test/java/org/apache/hadoop/hbase/types/TestPBCell.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/ByteBloomFilter.java hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/BaseTestRowData.java hbase-prefix-tree/src/test/java/org/apache/hadoop/hbase/codec/prefixtree/row/data/TestRowDataSimple.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileWriterV3.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestSeekTo.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStripeCompactor.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java hbase-common/src/main/java/org/apache/hadoop/hbase/util/test/RedundantKVGenerator.java hbase-server/src/test/java/org/apache/hadoop/hbase/TestSerialization.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellSkipListSet.java hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestResult.java hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java hbase-common/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java
        Hide
        stack added a comment -

        No harm in a release note on a big patch that has had a bunch of work expended on it.

        Should I file a blocker against 3.0 to remove KvComparator? or is it too early to do so?

        Sounds good to me

        ramkrishna.s.vasudevan

        Show
        stack added a comment - No harm in a release note on a big patch that has had a bunch of work expended on it. Should I file a blocker against 3.0 to remove KvComparator? or is it too early to do so? Sounds good to me ramkrishna.s.vasudevan
        Hide
        Duo Zhang added a comment -

        https://builds.apache.org/job/HBase-TRUNK/6456/findbugsResult/new/HIGH/

        Should be a bug?

        BufferedDataBlockEncoder.java
        if (comparator != null) {
          ...
        } else {
          Cell r = new KeyValue.KeyOnlyKeyValue(current.keyBuffer, 0, current.keyLength);
          comp = comparator.compareKeyIgnoresMvcc(seekCell, r);  // <==== NPE?
        }
        
        Show
        Duo Zhang added a comment - https://builds.apache.org/job/HBase-TRUNK/6456/findbugsResult/new/HIGH/ Should be a bug? BufferedDataBlockEncoder.java if (comparator != null ) { ... } else { Cell r = new KeyValue.KeyOnlyKeyValue(current.keyBuffer, 0, current.keyLength); comp = comparator.compareKeyIgnoresMvcc(seekCell, r); // <==== NPE? }
        Hide
        stack added a comment -

        Smile. ramkrishna.s.vasudevan... see above from Duo Zhang.

        Show
        stack added a comment - Smile. ramkrishna.s.vasudevan ... see above from Duo Zhang .
        Hide
        ramkrishna.s.vasudevan added a comment -

        Duo Zhang
        Thanks for pointing out. Even before this the else should have been a dead code. We can just remove that piece of code.
        Before patch
        See the constructor of BDE

            public BufferedEncodedSeeker(KVComparator comparator,
                HFileBlockDecodingContext decodingCtx) {
              this.comparator = comparator;
              this.samePrefixComparator = comparator;
              this.decodingCtx = decodingCtx;
        

        And this piece of code was

        if (samePrefixComparator != null) {
        ........
        else {
        Cell r = new KeyValue.KeyOnlyKeyValue(current.keyBuffer, 0, current.keyLength);
                  comp = comparator.compareOnlyKeyPortion(seekCell, r);
        }
        

        Ideally both were same. In this patch I removed this unnecessary two references. But forgot to clear this. I can given an addendum or raise a new JIRA for the this.

        Show
        ramkrishna.s.vasudevan added a comment - Duo Zhang Thanks for pointing out. Even before this the else should have been a dead code. We can just remove that piece of code. Before patch See the constructor of BDE public BufferedEncodedSeeker(KVComparator comparator, HFileBlockDecodingContext decodingCtx) { this .comparator = comparator; this .samePrefixComparator = comparator; this .decodingCtx = decodingCtx; And this piece of code was if (samePrefixComparator != null ) { ........ else { Cell r = new KeyValue.KeyOnlyKeyValue(current.keyBuffer, 0, current.keyLength); comp = comparator.compareOnlyKeyPortion(seekCell, r); } Ideally both were same. In this patch I removed this unnecessary two references. But forgot to clear this. I can given an addendum or raise a new JIRA for the this.
        Hide
        stack added a comment -

        ramkrishna.s.vasudevan Make new patch I'd say since it has a history. Link here.

        Show
        stack added a comment - ramkrishna.s.vasudevan Make new patch I'd say since it has a history. Link here.

          People

          • Assignee:
            ramkrishna.s.vasudevan
            Reporter:
            ramkrishna.s.vasudevan
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development