HBase
  1. HBase
  2. HBASE-11805

KeyValue to Cell Convert in WALEdit APIs

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.99.0, 2.0.0
    • Component/s: wal
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      The KeyValue based APIs in WALEdit is been removed and replaced with Cell based APIs
      void add(KeyValue) -> void add(Cell)
      ArrayList<KeyValue> getKeyValues() -> ArrayList<Cell> getCells()
      Show
      The KeyValue based APIs in WALEdit is been removed and replaced with Cell based APIs void add(KeyValue) -> void add(Cell) ArrayList<KeyValue> getKeyValues() -> ArrayList<Cell> getCells()

      Description

      In almost all other main interface class/APIs we have changed KeyValue to Cell. But missing in WALEdit. This is public marked for Replication (Well it should be for CP also)
      These 2 APIs deal with KVs
      add(KeyValue kv)
      ArrayList<KeyValue> getKeyValues()

      Suggest deprecate them and add for 0.98
      add(Cell kv)
      List<Cell> getCells()

      And just replace from 1.0

      1. HBASE-11805_0.98_addendum.patch
        30 kB
        Anoop Sam John
      2. HBASE-11805_V3.patch
        72 kB
        Anoop Sam John
      3. HBASE-11805_0.99.patch
        70 kB
        Anoop Sam John
      4. HBASE-11805_0.98_V2.patch
        56 kB
        Anoop Sam John
      5. HBASE-11805_0.98.patch
        54 kB
        Anoop Sam John
      6. HBASE-11805_V2.patch
        70 kB
        Anoop Sam John
      7. HBASE-11805.patch
        69 kB
        Anoop Sam John

        Issue Links

          Activity

          Hide
          Anoop Sam John added a comment -

          WALEdit implements HeapSize, and as Cell is not having heapSize() we will ending up in issue.
          Also will have to have the setSeqId() which is there only in KeyValue
          KeyValue#length() is the other one which is used by/from WALEdit.

          The background of this is, we need different type of Tag which only has to go into WAL, then if the WALEdit add takes KV only, we have to recreate a KV which needs byte copying for entire KV key, value etc. If we have WALEdit#add(Cell) we can create a new Cell impl where the Key and Value part refer to the same byte[] and tags part only needs a byte[] copy and recreation.

          Any ideas for the extension of Cell? stack, ramkrishna.s.vasudevan

          Show
          Anoop Sam John added a comment - WALEdit implements HeapSize, and as Cell is not having heapSize() we will ending up in issue. Also will have to have the setSeqId() which is there only in KeyValue KeyValue#length() is the other one which is used by/from WALEdit. The background of this is, we need different type of Tag which only has to go into WAL, then if the WALEdit add takes KV only, we have to recreate a KV which needs byte copying for entire KV key, value etc. If we have WALEdit#add(Cell) we can create a new Cell impl where the Key and Value part refer to the same byte[] and tags part only needs a byte[] copy and recreation. Any ideas for the extension of Cell? stack , ramkrishna.s.vasudevan
          Hide
          ramkrishna.s.vasudevan added a comment -

          In almost all other main interface class/APIs we have changed KeyValue to Cell.

          Still not in write path and in Memstores. Some of the problems mentioned in the comment above are applicable for those areas also. But in write path doing it would not be necessary. until we end up in a new format of hfileblock.

          we need different type of Tag which only has to go into WAL

          Okie.
          But in one way having WALEdit having KV is right becuase there is no seperated byte array for each of the row key components and so KeyValue would be the best fit here, but if Cell atleast the copying of tags/values could be done (as in this requirement). Cells would be of use when we have to do some copying but still reduce the copy that happens.
          We had a similar argument over in HBASE-7320 where there was a need to strip the tags without rewriting them. We still want that behaviour in places where the tags should not be sent to the client (atleast the tags of security related things should be stripped). We had exact same discussion there.

          Show
          ramkrishna.s.vasudevan added a comment - In almost all other main interface class/APIs we have changed KeyValue to Cell. Still not in write path and in Memstores. Some of the problems mentioned in the comment above are applicable for those areas also. But in write path doing it would not be necessary. until we end up in a new format of hfileblock. we need different type of Tag which only has to go into WAL Okie. But in one way having WALEdit having KV is right becuase there is no seperated byte array for each of the row key components and so KeyValue would be the best fit here, but if Cell atleast the copying of tags/values could be done (as in this requirement). Cells would be of use when we have to do some copying but still reduce the copy that happens. We had a similar argument over in HBASE-7320 where there was a need to strip the tags without rewriting them. We still want that behaviour in places where the tags should not be sent to the client (atleast the tags of security related things should be stripped). We had exact same discussion there.
          Hide
          Anoop Sam John added a comment -

          I mean in the Exposed APIs/Interfaces we have Cell now. WALEdit is exposed.. (Limited for replication and for CP also)

          Show
          Anoop Sam John added a comment - I mean in the Exposed APIs/Interfaces we have Cell now. WALEdit is exposed.. (Limited for replication and for CP also)
          Hide
          stack added a comment -

          WALEdit implements HeapSize, and as Cell is not having heapSize() we will ending up in issue.

          CellUtil does heap size? CellUtil#estimateSizeOf

          Also will have to have the setSeqId() which is there only in KeyValue

          Man. Its a pity. Cell is all getters. Would be shame adding in this one setter. So, let implementations implement Cell and SequenceNumber (add setSequenceNumber to the SequenceNumber Interface) on the server side?

          KeyValue#length() is the other one which is used by/from WALEdit.

          Is that because WALEdit is doing it wrong? Is this how it preallocates and copies over KV?

          This is low-priority Anoop Sam John (as per Ram) because we are working on read-path first?

          Show
          stack added a comment - WALEdit implements HeapSize, and as Cell is not having heapSize() we will ending up in issue. CellUtil does heap size? CellUtil#estimateSizeOf Also will have to have the setSeqId() which is there only in KeyValue Man. Its a pity. Cell is all getters. Would be shame adding in this one setter. So, let implementations implement Cell and SequenceNumber (add setSequenceNumber to the SequenceNumber Interface) on the server side? KeyValue#length() is the other one which is used by/from WALEdit. Is that because WALEdit is doing it wrong? Is this how it preallocates and copies over KV? This is low-priority Anoop Sam John (as per Ram) because we are working on read-path first?
          Hide
          ramkrishna.s.vasudevan added a comment -

          This is low-priority Anoop Sam John (as per Ram) because we are working on read-path first?

          Stack, this change though in the right path tries to achieve some other use case by converting it into cell. So if we are able to do this it would help in cases as Anoop mentions above

          the background of this is, we need different type of Tag which only has to go into WAL,

          Show
          ramkrishna.s.vasudevan added a comment - This is low-priority Anoop Sam John (as per Ram) because we are working on read-path first? Stack, this change though in the right path tries to achieve some other use case by converting it into cell. So if we are able to do this it would help in cases as Anoop mentions above the background of this is, we need different type of Tag which only has to go into WAL,
          Hide
          Anoop Sam John added a comment -

          Man. Its a pity. Cell is all getters. Would be shame adding in this one setter. So, let implementations implement Cell and SequenceNumber (add setSequenceNumber to the SequenceNumber Interface) on the server side?

          I was just saying the issues I met with once I change KV to Cell. Not saying we should add setter. I am also think that is not correct. Sorry my statement was not clear.

          This is low-priority Anoop Sam John (as per Ram) because we are working on read-path first?

          This is not just wrt offheap work. WALEdit is exposed interface now. For Replication and CP atleast. So I was thinking that we should finalize the API in that atleast before 1.0.

          Show
          Anoop Sam John added a comment - Man. Its a pity. Cell is all getters. Would be shame adding in this one setter. So, let implementations implement Cell and SequenceNumber (add setSequenceNumber to the SequenceNumber Interface) on the server side? I was just saying the issues I met with once I change KV to Cell. Not saying we should add setter. I am also think that is not correct. Sorry my statement was not clear. This is low-priority Anoop Sam John (as per Ram) because we are working on read-path first? This is not just wrt offheap work. WALEdit is exposed interface now. For Replication and CP atleast. So I was thinking that we should finalize the API in that atleast before 1.0.
          Hide
          stack added a comment -

          Agree.

          We need the setter don't we?

          Show
          stack added a comment - Agree. We need the setter don't we?
          Hide
          Anoop Sam John added a comment -

          CellUtil does heap size? CellUtil#estimateSizeOf

          That is not heapSize but the length(). Wherever we use WALEdit KVs legth() I use CellUtil helper API now. May be heapSize also similar way adding some thing in CellUtil.

          Again my main intent is to clean the API with using Cell instead of KV. Implementation wise we can correct/better later also. But the API signature can get changed before 1.0 (IMHO)

          Show
          Anoop Sam John added a comment - CellUtil does heap size? CellUtil#estimateSizeOf That is not heapSize but the length(). Wherever we use WALEdit KVs legth() I use CellUtil helper API now. May be heapSize also similar way adding some thing in CellUtil. Again my main intent is to clean the API with using Cell instead of KV. Implementation wise we can correct/better later also. But the API signature can get changed before 1.0 (IMHO)
          Hide
          Anoop Sam John added a comment - - edited

          We need the setter don't we?

          Yes but with new Interface. I was thinking initially to have an Interface extending Cell and using that in server side. But what u suggested looks better

          SequenceNumber (add setSequenceNumber to the SequenceNumber Interface)

          If the passed in Object is of type SequenceNumber we can call setter directly. If not, there is no other go but to recreate a KV and set seqNo

          Show
          Anoop Sam John added a comment - - edited We need the setter don't we? Yes but with new Interface. I was thinking initially to have an Interface extending Cell and using that in server side. But what u suggested looks better SequenceNumber (add setSequenceNumber to the SequenceNumber Interface) If the passed in Object is of type SequenceNumber we can call setter directly. If not, there is no other go but to recreate a KV and set seqNo
          Hide
          stack added a comment -

          You are right. heapsize != length. Need to add comment to estimateSizeOf. estimateSizeOf is not the serialized size either. Its estimate. We have to give up on heapSize for cell and use estimates instead? There was discussion in an old issue on heapsize and Cell and was thought we should not add it unless really needed and wasn't thought needed at the time. See HBASE-9383. We've been here before (smile).

          Show
          stack added a comment - You are right. heapsize != length. Need to add comment to estimateSizeOf. estimateSizeOf is not the serialized size either. Its estimate. We have to give up on heapSize for cell and use estimates instead? There was discussion in an old issue on heapsize and Cell and was thought we should not add it unless really needed and wasn't thought needed at the time. See HBASE-9383 . We've been here before (smile).
          Hide
          ramkrishna.s.vasudevan added a comment -

          Cells having SequenceNumber is also fine. HBASE-11777 deals with it.

          Show
          ramkrishna.s.vasudevan added a comment - Cells having SequenceNumber is also fine. HBASE-11777 deals with it.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Can we atleast implement length() or something similar to the Cell API then?

          Show
          ramkrishna.s.vasudevan added a comment - Can we atleast implement length() or something similar to the Cell API then?
          Hide
          stack added a comment -

          What would length mean? If its prefix encoded or compressed, is it length of the encoded/compressed Cell or do we expand and then return the length?

          Show
          stack added a comment - What would length mean? If its prefix encoded or compressed, is it length of the encoded/compressed Cell or do we expand and then return the length?
          Hide
          Andrew Purtell added a comment -

          Moving out of .6

          Show
          Andrew Purtell added a comment - Moving out of .6
          Hide
          Anoop Sam John added a comment -

          Patch is ready with me but depends on HBASE-11777. Once that is committed will attach here for review.

          Show
          Anoop Sam John added a comment - Patch is ready with me but depends on HBASE-11777 . Once that is committed will attach here for review.
          Hide
          Anoop Sam John added a comment -

          Enis Soztutar I would like to see this into 0.99.0 Will give a patch very soon.

          Show
          Anoop Sam John added a comment - Enis Soztutar I would like to see this into 0.99.0 Will give a patch very soon.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 45 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 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/10681//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//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/12665959/HBASE-11805.patch against trunk revision . ATTACHMENT ID: 12665959 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 45 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 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/10681//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10681//console This message is automatically generated.
          Hide
          Enis Soztutar added a comment -

          I would like to see this into 0.99.0 Will give a patch very soon.

          Sounds good. Will wait for the final patch.

          Show
          Enis Soztutar added a comment - I would like to see this into 0.99.0 Will give a patch very soon. Sounds good. Will wait for the final patch.
          Hide
          Anoop Sam John added a comment -

          Thanks Enis Soztutar. Pls have a look at the attached patch which has to go in for branch-1 also. (Removing the APIs dealing with KV and keeping only Cell)
          For 0.98 we will keep the KV based APIs with deprecation.

          Show
          Anoop Sam John added a comment - Thanks Enis Soztutar . Pls have a look at the attached patch which has to go in for branch-1 also. (Removing the APIs dealing with KV and keeping only Cell) For 0.98 we will keep the KV based APIs with deprecation.
          Hide
          stack added a comment -

          Do we have to do the below?

          • for (KeyValue kv : value.getKeyValues()) {
            + for (Cell cell : value.getCells()) {
            + KeyValue kv = KeyValueUtil.ensureKeyValue(cell);

          Maybe we could make do w/ a Cell? Seems like later changes have determined that you don't need KV so you have probably already tried my suggestion and it just don't work here... thats fine. Just asking.

          This one is a bit odd Anoop...

          • for (KeyValue kv: kvs) {
          • size += kv.getLength();
            + for (Cell cell: cells) { + size += KeyValueUtil.length(cell); }

          Using a KeyValueUtils on a cell?

          Nice one here:

          • walEdit.add(KeyValueUtil.ensureKeyValue(cell));
            + walEdit.add(cell);

          Good

          This too....

          • kv.setSequenceId(currentReplaySeqId);
            + CellUtil.setSequenceId(cell, currentReplaySeqId);

          +1 on patch. A few nits above. Nice stuff.

          Show
          stack added a comment - Do we have to do the below? for (KeyValue kv : value.getKeyValues()) { + for (Cell cell : value.getCells()) { + KeyValue kv = KeyValueUtil.ensureKeyValue(cell); Maybe we could make do w/ a Cell? Seems like later changes have determined that you don't need KV so you have probably already tried my suggestion and it just don't work here... thats fine. Just asking. This one is a bit odd Anoop... for (KeyValue kv: kvs) { size += kv.getLength(); + for (Cell cell: cells) { + size += KeyValueUtil.length(cell); } Using a KeyValueUtils on a cell? Nice one here: walEdit.add(KeyValueUtil.ensureKeyValue(cell)); + walEdit.add(cell); Good This too.... kv.setSequenceId(currentReplaySeqId); + CellUtil.setSequenceId(cell, currentReplaySeqId); +1 on patch. A few nits above. Nice stuff.
          Hide
          Anoop Sam John added a comment -

          Thanks Stack.

          Do we have to do the below?
          
             - for (KeyValue kv : value.getKeyValues()) {
              + for (Cell cell : value.getCells()) {
              + KeyValue kv = KeyValueUtil.ensureKeyValue(cell);
          

          This is in WALPlayer#HLogKeyValueMapper. The mapper o/p value is KeyValue. This also we can change to Cell. But I thought of doing all these as part of HBASE-11871. One sub task to remove enusreKeyValue from MR tools. Sounds ok?

          This one is a bit odd Anoop...

              -for (KeyValue kv: kvs) {
              -size += kv.getLength();
              + for (Cell cell: cells) { + size += KeyValueUtil.length(cell); }
          

          Using a KeyValueUtils on a cell?

          We have CellUtil#estimatedSizeOf() but that is not KV length. That is length + SIZEOF_INT. The bytes a Cell takes when serialized to Encoder.
          KKUtil method was existing and been used by Prefix Tree also. May be will add length() in CellUtil , same way as in KVUtil and make code to use that.

          Show
          Anoop Sam John added a comment - Thanks Stack. Do we have to do the below? - for (KeyValue kv : value.getKeyValues()) { + for (Cell cell : value.getCells()) { + KeyValue kv = KeyValueUtil.ensureKeyValue(cell); This is in WALPlayer#HLogKeyValueMapper. The mapper o/p value is KeyValue. This also we can change to Cell. But I thought of doing all these as part of HBASE-11871 . One sub task to remove enusreKeyValue from MR tools. Sounds ok? This one is a bit odd Anoop... - for (KeyValue kv: kvs) { -size += kv.getLength(); + for (Cell cell: cells) { + size += KeyValueUtil.length(cell); } Using a KeyValueUtils on a cell? We have CellUtil#estimatedSizeOf() but that is not KV length. That is length + SIZEOF_INT. The bytes a Cell takes when serialized to Encoder. KKUtil method was existing and been used by Prefix Tree also. May be will add length() in CellUtil , same way as in KVUtil and make code to use that.
          Hide
          stack added a comment -

          Sounds reasonable. +1

          Show
          stack added a comment - Sounds reasonable. +1
          Hide
          Anoop Sam John added a comment -

          V2 addressing Stack's comment.
          Added CellUtil#estimatedLengthOf(final Cell cell)
          When passed cell is a KV we return the KV.length() to maintain the same result as in the past. When cell is of other types, the return will be an estimate adding up the key, value and tags lengths part.

          Show
          Anoop Sam John added a comment - V2 addressing Stack's comment. Added CellUtil#estimatedLengthOf(final Cell cell) When passed cell is a KV we return the KV.length() to maintain the same result as in the past. When cell is of other types, the return will be an estimate adding up the key, value and tags lengths part.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 45 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 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.master.TestMasterFailover
          org.apache.hadoop.hbase.client.TestMultiParallel
          org.apache.hadoop.hbase.client.TestReplicaWithCluster
          org.apache.hadoop.hbase.replication.TestPerTableCFReplication

          -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScanBase.testScan(TestTableInputFormatScanBase.java:238)
          at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScan1.testScanEmptyToBBA(TestTableInputFormatScan1.java:70)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//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/12666280/HBASE-11805_V2.patch against trunk revision . ATTACHMENT ID: 12666280 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 45 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 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.master.TestMasterFailover org.apache.hadoop.hbase.client.TestMultiParallel org.apache.hadoop.hbase.client.TestReplicaWithCluster org.apache.hadoop.hbase.replication.TestPerTableCFReplication -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScanBase.testScan(TestTableInputFormatScanBase.java:238) at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScan1.testScanEmptyToBBA(TestTableInputFormatScan1.java:70) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10696//console This message is automatically generated.
          Hide
          Anoop Sam John added a comment -

          0.98 version of the patch. Ping Andrew Purtell
          Same trunk patch has to go into branch-1 as well. Ping Enis Soztutar

          Show
          Anoop Sam John added a comment - 0.98 version of the patch. Ping Andrew Purtell Same trunk patch has to go into branch-1 as well. Ping Enis Soztutar
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12666448/HBASE-11805_0.98.patch
          against trunk revision .
          ATTACHMENT ID: 12666448

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10711//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/12666448/HBASE-11805_0.98.patch against trunk revision . ATTACHMENT ID: 12666448 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 24 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10711//console This message is automatically generated.
          Hide
          Andrew Purtell added a comment -

          Looks good, except this:

          +@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC)
           public class WALEdit implements Writable, HeapSize {
          

          This might conflict with one of the JIRAs adding these annotations.

          Leave this hunk out and +1

          Show
          Andrew Purtell added a comment - Looks good, except this: +@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC) public class WALEdit implements Writable, HeapSize { This might conflict with one of the JIRAs adding these annotations. Leave this hunk out and +1
          Hide
          ramkrishna.s.vasudevan added a comment -

          There are codes like this

              while (kvs.size() < expectedCount && cellDecoder.advance()) {
                Cell cell = cellDecoder.current();
                if (!(cell instanceof KeyValue)) {
                  throw new IOException("WAL edit only supports KVs as cells");
                }
                kvs.add((KeyValue)cell);
              }
          

          We may need to check these places. Discussing wth Anoop here, if we try to work with cells and try using cells in walEdit unless the codec in the replication is changed to work with Cells we don't get the real benefit of Cells in waledit. But it is always right to change all our data structures to deal with Cells rather than Kvs.

          Show
          ramkrishna.s.vasudevan added a comment - There are codes like this while (kvs.size() < expectedCount && cellDecoder.advance()) { Cell cell = cellDecoder.current(); if (!(cell instanceof KeyValue)) { throw new IOException( "WAL edit only supports KVs as cells" ); } kvs.add((KeyValue)cell); } We may need to check these places. Discussing wth Anoop here, if we try to work with cells and try using cells in walEdit unless the codec in the replication is changed to work with Cells we don't get the real benefit of Cells in waledit. But it is always right to change all our data structures to deal with Cells rather than Kvs.
          Hide
          Anoop Sam John added a comment -

          My bad. I missed reading that code. Thanks we will have to remove.

          Yes to get the adv we must handle the Codec side also. That is why the Umbrella jira I raised (HBASE-11871) Will check and raise a sub task to use the Cells in Codec where we write the WALEdit.

          One main intent for this Jira is to change the exposed interface signature to use Cell. This has to happen before 1.0 release. WALEdit is exposed to CPs and Replication.

          Thanks for the review Ram.

          Show
          Anoop Sam John added a comment - My bad. I missed reading that code. Thanks we will have to remove. Yes to get the adv we must handle the Codec side also. That is why the Umbrella jira I raised ( HBASE-11871 ) Will check and raise a sub task to use the Cells in Codec where we write the WALEdit. One main intent for this Jira is to change the exposed interface signature to use Cell. This has to happen before 1.0 release. WALEdit is exposed to CPs and Replication. Thanks for the review Ram.
          Hide
          ramkrishna.s.vasudevan added a comment -

          That is why the Umbrella jira I raised (HBASE-11871) Will check and raise a sub task to use the Cells in Codec where we write the WALEdit.

          One main intent for this Jira is to change the exposed interface signature to use Cell. This has to happen before 1.0 release. WALEdit is exposed to CPs and Replication.

          +1

          Show
          ramkrishna.s.vasudevan added a comment - That is why the Umbrella jira I raised ( HBASE-11871 ) Will check and raise a sub task to use the Cells in Codec where we write the WALEdit. One main intent for this Jira is to change the exposed interface signature to use Cell. This has to happen before 1.0 release. WALEdit is exposed to CPs and Replication. +1
          Hide
          Anoop Sam John added a comment -

          Andrew Purtell

          +@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC)
           public class WALEdit implements Writable, HeapSize {

          The Jira adding the annotations is in now. WALEdit is still private. But it is exposed to CPs via different hooks. So I am changing it to LimitedPrivate for COPROC

          V3 patch solves comments from Ram and Stack. Will commit tomorrow unless objections. Hope you ok Enis Soztutar

          Show
          Anoop Sam John added a comment - Andrew Purtell +@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC) public class WALEdit implements Writable, HeapSize { The Jira adding the annotations is in now. WALEdit is still private. But it is exposed to CPs via different hooks. So I am changing it to LimitedPrivate for COPROC V3 patch solves comments from Ram and Stack. Will commit tomorrow unless objections. Hope you ok Enis Soztutar
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 45 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 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.TestRegionRebalancing

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//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/12666998/HBASE-11805_V3.patch against trunk revision . ATTACHMENT ID: 12666998 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 45 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 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.TestRegionRebalancing Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10733//console This message is automatically generated.
          Hide
          stack added a comment -

          +1 on re-skim. Looks safe.

          Show
          stack added a comment - +1 on re-skim. Looks safe.
          Hide
          Anoop Sam John added a comment -

          Pushed to 0.98+. Thanks for the review Stack, Andy, Ram.

          Show
          Anoop Sam John added a comment - Pushed to 0.98+. Thanks for the review Stack, Andy, Ram.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.98 #502 (See https://builds.apache.org/job/HBase-0.98/502/)
          HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev 3f6a44b98cdf4a6dc28a713bc70e4f5bb03ff36e)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.98 #502 (See https://builds.apache.org/job/HBase-0.98/502/ ) HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev 3f6a44b98cdf4a6dc28a713bc70e4f5bb03ff36e) hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-1.0 #160 (See https://builds.apache.org/job/HBase-1.0/160/)
          HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev ecb015d9a3c7d0aa0320670674cc1e55f390373f)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ScopeWALEntryFilter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/HLogPerformanceEvaluation.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/TableCfWALEntryFilter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationEndpoint.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationWALEntryFilters.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-1.0 #160 (See https://builds.apache.org/job/HBase-1.0/160/ ) HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev ecb015d9a3c7d0aa0320670674cc1e55f390373f) hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ScopeWALEntryFilter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/HLogPerformanceEvaluation.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/TableCfWALEntryFilter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationEndpoint.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationWALEntryFilters.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-TRUNK #5475 (See https://builds.apache.org/job/HBase-TRUNK/5475/)
          HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev 0a9bfcaf744a908c1710fec47de073724ec2d8be)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/TableCfWALEntryFilter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/HLogPerformanceEvaluation.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ScopeWALEntryFilter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestRegionReplicaReplicationEndpointNoMaster.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationWALEntryFilters.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationEndpoint.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/RegionReplicaReplicationEndpoint.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #5475 (See https://builds.apache.org/job/HBase-TRUNK/5475/ ) HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev 0a9bfcaf744a908c1710fec47de073724ec2d8be) hbase-server/src/main/java/org/apache/hadoop/hbase/replication/TableCfWALEntryFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/HLogPerformanceEvaluation.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ScopeWALEntryFilter.java hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestRegionReplicaReplicationEndpointNoMaster.java hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationWALEntryFilters.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationEndpoint.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/RegionReplicaReplicationEndpoint.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #476 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/476/)
          HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev 3f6a44b98cdf4a6dc28a713bc70e4f5bb03ff36e)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #476 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/476/ ) HBASE-11805 KeyValue to Cell Convert in WALEdit APIs. (anoopsamjohn: rev 3f6a44b98cdf4a6dc28a713bc70e4f5bb03ff36e) hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          Hide
          Lars Hofhansl added a comment -

          Looks like this potentially breaks Phoenix (and maybe other things) with exceptions like these:

          java.lang.NullPointerException
                  at org.apache.hadoop.hbase.util.Bytes.toShort(Bytes.java:845)
                  at org.apache.hadoop.hbase.util.Bytes.toShort(Bytes.java:832)
                  at org.apache.hadoop.hbase.KeyValue.getRowLength(KeyValue.java:1303)
                  at org.apache.hadoop.hbase.KeyValue.getFamilyOffset(KeyValue.java:1319)
                  at org.apache.hadoop.hbase.KeyValue.getFamilyLength(KeyValue.java:1334)
                  at org.apache.hadoop.hbase.CellUtil.cloneFamily(CellUtil.java:70)
                  at org.apache.hadoop.hbase.KeyValue.getFamily(KeyValue.java:1559)
                  at org.apache.hadoop.hbase.replication.regionserver.Replication.scopeWAL
          Edits(Replication.java:249)
                  at org.apache.hadoop.hbase.replication.regionserver.Replication.visitLog
          EntryBeforeWrite(Replication.java:233)
                  at org.apache.hadoop.hbase.regionserver.wal.FSHLog.doWrite(FSHLog.java:1486)
                  at org.apache.hadoop.hbase.regionserver.wal.FSHLog.append(FSHLog.java:1023)
                  at org.apache.hadoop.hbase.regionserver.wal.FSHLog.appendNoSync(FSHLog.java:1054)
                  at org.apache.hadoop.hbase.regionserver.HRegion.doMiniBatchMutation(HRegion.java:2553)
          
          Show
          Lars Hofhansl added a comment - Looks like this potentially breaks Phoenix (and maybe other things) with exceptions like these: java.lang.NullPointerException at org.apache.hadoop.hbase.util.Bytes.toShort(Bytes.java:845) at org.apache.hadoop.hbase.util.Bytes.toShort(Bytes.java:832) at org.apache.hadoop.hbase.KeyValue.getRowLength(KeyValue.java:1303) at org.apache.hadoop.hbase.KeyValue.getFamilyOffset(KeyValue.java:1319) at org.apache.hadoop.hbase.KeyValue.getFamilyLength(KeyValue.java:1334) at org.apache.hadoop.hbase.CellUtil.cloneFamily(CellUtil.java:70) at org.apache.hadoop.hbase.KeyValue.getFamily(KeyValue.java:1559) at org.apache.hadoop.hbase.replication.regionserver.Replication.scopeWAL Edits(Replication.java:249) at org.apache.hadoop.hbase.replication.regionserver.Replication.visitLog EntryBeforeWrite(Replication.java:233) at org.apache.hadoop.hbase.regionserver.wal.FSHLog.doWrite(FSHLog.java:1486) at org.apache.hadoop.hbase.regionserver.wal.FSHLog.append(FSHLog.java:1023) at org.apache.hadoop.hbase.regionserver.wal.FSHLog.appendNoSync(FSHLog.java:1054) at org.apache.hadoop.hbase.regionserver.HRegion.doMiniBatchMutation(HRegion.java:2553)
          Hide
          Lars Hofhansl added a comment -

          Tried with 0.98 current build compared to a build of the 0.98.6RC2 tag. The former build shows this, the latter does not.
          Anoop Sam John, Andrew Purtell, FYI.

          Show
          Lars Hofhansl added a comment - Tried with 0.98 current build compared to a build of the 0.98.6RC2 tag. The former build shows this, the latter does not. Anoop Sam John , Andrew Purtell , FYI.
          Hide
          Anoop Sam John added a comment -

          Checking this Lars Hofhansl
          Looks strange that in a KeyValue object we have a null ref to bytes array!

          Show
          Anoop Sam John added a comment - Checking this Lars Hofhansl Looks strange that in a KeyValue object we have a null ref to bytes array!
          Hide
          Anoop Sam John added a comment -

          Reading the Phoenix code, I think I got the issue.

          WALEdit edit = miniBatchOp.getWalEdit(i);
                // we don't have a WALEdit for immutable index cases, which still see this path
                // we could check is indexing is enable for the mutation in prePut and then just skip this
                // after checking here, but this saves us the checking again.
                if (edit != null) {
                  KeyValue kv = edit.getKeyValues().get(0);
                  if (kv == BATCH_MARKER) {
                    // remove batch marker from the WALEdit
                    edit.getKeyValues().remove(0);
                  }
                }
          

          BATCH_MARKER is a KV with null bytes. This removal wont happen now as we return a new ArrayList from WALEdit#getKeyValues()
          Thinking abt how we can solve.

          Show
          Anoop Sam John added a comment - Reading the Phoenix code, I think I got the issue. WALEdit edit = miniBatchOp.getWalEdit(i); // we don't have a WALEdit for immutable index cases, which still see this path // we could check is indexing is enable for the mutation in prePut and then just skip this // after checking here, but this saves us the checking again. if (edit != null ) { KeyValue kv = edit.getKeyValues().get(0); if (kv == BATCH_MARKER) { // remove batch marker from the WALEdit edit.getKeyValues().remove(0); } } BATCH_MARKER is a KV with null bytes. This removal wont happen now as we return a new ArrayList from WALEdit#getKeyValues() Thinking abt how we can solve.
          Hide
          Anoop Sam John added a comment -

          I logged PHOENIX-1245 to handle the issue in Phoenix side. FYI Lars Hofhansl

          Show
          Anoop Sam John added a comment - I logged PHOENIX-1245 to handle the issue in Phoenix side. FYI Lars Hofhansl
          Hide
          Andrew Purtell added a comment -

          now as we return a new ArrayList from WALEdit#getKeyValues()

          Isn't this an API regression? The client code expects to be able to use the returned list to change the WALEdit.

          Show
          Andrew Purtell added a comment - now as we return a new ArrayList from WALEdit#getKeyValues() Isn't this an API regression? The client code expects to be able to use the returned list to change the WALEdit.
          Hide
          Andrew Purtell added a comment -

          Reopening.

          Show
          Andrew Purtell added a comment - Reopening.
          Hide
          Lars Hofhansl added a comment -

          Yeah, that seems like a (implicit albeit) API regression. Is there a way to keep the identity of KVs?

          Show
          Lars Hofhansl added a comment - Yeah, that seems like a (implicit albeit) API regression. Is there a way to keep the identity of KVs?
          Hide
          Anoop Sam John added a comment -

          Even though it is a bit ugly way to deal with the internal data structure of WALEdit and tried removing from that, we have to solve this issue. From the getter we have given back the original data structure and one can play with that. We have add API in WALEdit and one can add with out calling this but just by getting the List and adding directly to that. Remove is also like that. The ideal way would have been to ask for a remove() API.

          We can solve this by changing 0.98 code such that we keep the cells in the form of KeyValues in WALEdit. And the getter will continue to return the original list so that any removal will actually remove the Cell(KV) from WALEdit. 0.98 scope will get reduced to just deprecate the APIs and so adding the new one. The new one also will just delegate call to the older one. The 98 internal code path also will continue to deal with KeyValue based APIs. Pls confirm whether this is ok Andrew Purtell.

          Show
          Anoop Sam John added a comment - Even though it is a bit ugly way to deal with the internal data structure of WALEdit and tried removing from that, we have to solve this issue. From the getter we have given back the original data structure and one can play with that. We have add API in WALEdit and one can add with out calling this but just by getting the List and adding directly to that. Remove is also like that. The ideal way would have been to ask for a remove() API. We can solve this by changing 0.98 code such that we keep the cells in the form of KeyValues in WALEdit. And the getter will continue to return the original list so that any removal will actually remove the Cell(KV) from WALEdit. 0.98 scope will get reduced to just deprecate the APIs and so adding the new one. The new one also will just delegate call to the older one. The 98 internal code path also will continue to deal with KeyValue based APIs. Pls confirm whether this is ok Andrew Purtell .
          Hide
          Lars Hofhansl added a comment -

          It's fine to return a KV as a KV is a Cell. The question is: Are we locking our to always requiring support for KVs? As far as I understand we just need to return same the objects that were added to edit (whether they are Cells or KVs)

          Show
          Lars Hofhansl added a comment - It's fine to return a KV as a KV is a Cell. The question is: Are we locking our to always requiring support for KVs? As far as I understand we just need to return same the objects that were added to edit (whether they are Cells or KVs)
          Hide
          Anoop Sam John added a comment - - edited

          As part of this change I changed the internal data structure to
          ArrayList<Cell> cells
          from
          ArrayList<KeyValue> kvs
          We have to return the internal data structure same ref so that remove from the list will result in actual removal from WALEdit.
          That is why from old getKeyValues() API we had to return a newly created ArrayList and putting the cells (after KVUtil.ensureKeyValue())

          Now to fix, we will have to keep ArrayList as KeyValue generic and when adding to WALEdit we can add only KeyValue types. Then only we can return the same list ref. Am I making it clear LarsH?

          Show
          Anoop Sam John added a comment - - edited As part of this change I changed the internal data structure to ArrayList<Cell> cells from ArrayList<KeyValue> kvs We have to return the internal data structure same ref so that remove from the list will result in actual removal from WALEdit. That is why from old getKeyValues() API we had to return a newly created ArrayList and putting the cells (after KVUtil.ensureKeyValue()) Now to fix, we will have to keep ArrayList as KeyValue generic and when adding to WALEdit we can add only KeyValue types. Then only we can return the same list ref. Am I making it clear LarsH?
          Hide
          ramkrishna.s.vasudevan added a comment -

          I think we could actually revert this change in 0.98 alone for now? Or go with deprecating the API so that in the future releases we are able to handle with the new API and thus the dependent project can also know about these behaviourial changes?

          Show
          ramkrishna.s.vasudevan added a comment - I think we could actually revert this change in 0.98 alone for now? Or go with deprecating the API so that in the future releases we are able to handle with the new API and thus the dependent project can also know about these behaviourial changes?
          Hide
          Anoop Sam John added a comment -

          In 0.99+ we have removed the old KV based APIs and kept only Cell based APIs. In order to do that, we have to add the deprecation now and so the addition of the new APIs. But the changes should be only that much. So it is 90% revert. Does that make sense? I can make a new patch fro this and we can get that in to 98

          Show
          Anoop Sam John added a comment - In 0.99+ we have removed the old KV based APIs and kept only Cell based APIs. In order to do that, we have to add the deprecation now and so the addition of the new APIs. But the changes should be only that much. So it is 90% revert. Does that make sense? I can make a new patch fro this and we can get that in to 98
          Hide
          Lars Hofhansl added a comment -

          +1 on reverting this from 0.98 and adding the deprecation now.

          Show
          Lars Hofhansl added a comment - +1 on reverting this from 0.98 and adding the deprecation now.
          Hide
          Anoop Sam John added a comment -

          Patch to revert the changes. In effect now in 0.98 the code level changes are
          1. Addition of 2 new Cell based APIs in WALEdit
          2. Deprecated APIs getKeyValues() and add(KeyValue)

          Show
          Anoop Sam John added a comment - Patch to revert the changes. In effect now in 0.98 the code level changes are 1. Addition of 2 new Cell based APIs in WALEdit 2. Deprecated APIs getKeyValues() and add(KeyValue)
          Hide
          Anoop Sam John added a comment -

          Ping Andrew Purtell . Can you review pls so that we can get this in.

          Show
          Anoop Sam John added a comment - Ping Andrew Purtell . Can you review pls so that we can get this in.
          Hide
          Andrew Purtell added a comment - - edited

          Is the addendum patch the revert? Can we just revert this in SCM and then propose in a new JIRA the desired changes? We can take 0.98 out of the fix versions here and resolve this.
          Edit: An addendum patch that reverts a previous change while introducing new changes is hard to track, let's not do that.

          Show
          Andrew Purtell added a comment - - edited Is the addendum patch the revert? Can we just revert this in SCM and then propose in a new JIRA the desired changes? We can take 0.98 out of the fix versions here and resolve this. Edit: An addendum patch that reverts a previous change while introducing new changes is hard to track, let's not do that.
          Hide
          Anoop Sam John added a comment -

          Agree Andy. I have reverted the old commit (3f6a44b98cdf4a6dc28a713bc70e4f5bb03ff36e) for this from 0.98 branch.

          Show
          Anoop Sam John added a comment - Agree Andy. I have reverted the old commit (3f6a44b98cdf4a6dc28a713bc70e4f5bb03ff36e) for this from 0.98 branch.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #490 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/490/)
          Revert "HBASE-11805 KeyValue to Cell Convert in WALEdit APIs." (anoopsamjohn: rev 55a790e348c40002026fb5f2c1f63e5317964c68)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #490 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/490/ ) Revert " HBASE-11805 KeyValue to Cell Convert in WALEdit APIs." (anoopsamjohn: rev 55a790e348c40002026fb5f2c1f63e5317964c68) hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.98 #516 (See https://builds.apache.org/job/HBase-0.98/516/)
          Revert "HBASE-11805 KeyValue to Cell Convert in WALEdit APIs." (anoopsamjohn: rev 55a790e348c40002026fb5f2c1f63e5317964c68)

          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.98 #516 (See https://builds.apache.org/job/HBase-0.98/516/ ) Revert " HBASE-11805 KeyValue to Cell Convert in WALEdit APIs." (anoopsamjohn: rev 55a790e348c40002026fb5f2c1f63e5317964c68) hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #491 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/491/)
          Revert "HBASE-11805 KeyValue to Cell Convert in WALEdit APIs." (anoopsamjohn: rev 55a790e348c40002026fb5f2c1f63e5317964c68)

          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #491 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/491/ ) Revert " HBASE-11805 KeyValue to Cell Convert in WALEdit APIs." (anoopsamjohn: rev 55a790e348c40002026fb5f2c1f63e5317964c68) hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/ReplicationProtbufUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SampleRegionWALObserver.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestSecureHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogPrettyPrinter.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEditsReplaySink.java hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/WALPlayer.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHLogRecordReader.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogWriter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
          Hide
          Enis Soztutar added a comment -

          Closing this issue after 0.99.0 release.

          Show
          Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

            People

            • Assignee:
              Anoop Sam John
              Reporter:
              Anoop Sam John
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development