Details

    • Type: Sub-task Sub-task
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 2.0.0
    • Component/s: None
    • Labels:
      None
    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_2.patch
      201 kB
      ramkrishna.s.vasudevan
    8. HBASE-10800_18.patch
      389 kB
      ramkrishna.s.vasudevan
    9. HBASE-10800_16.patch
      388 kB
      ramkrishna.s.vasudevan
    10. HBASE-10800_15.patch
      387 kB
      ramkrishna.s.vasudevan
    11. HBASE-10800_14.patch
      387 kB
      ramkrishna.s.vasudevan
    12. HBASE-10800_13.patch
      388 kB
      ramkrishna.s.vasudevan
    13. HBASE-10800_12.patch
      392 kB
      ramkrishna.s.vasudevan
    14. HBASE-10800_11.patch
      391 kB
      ramkrishna.s.vasudevan
    15. 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
        zhangduo 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
        zhangduo 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
        zhangduo 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
        zhangduo 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
        zhangduo added a comment -

        So we decide to get rid of ByteRange?

        Show
        zhangduo 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 )

          People

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

            Dates

            • Created:
              Updated:

              Development