From 99512abc2e1b2d706bb3288fb49dab66cad9d7ff Mon Sep 17 00:00:00 2001 From: "jeongmin.kim" Date: Tue, 29 Mar 2022 22:42:06 +0900 Subject: [PATCH 1/3] HBASE-26901 delete with null columnQualifier occurs NullPointerException when NewVersionBehavior is on --- .../querymatcher/NewVersionBehaviorTracker.java | 16 +++++++++++----- .../TestNewVersionBehaviorFromClientSide.java | 12 +++++++++++- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java index 3c9a541d0b38..6bbb1ebb205b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java @@ -165,9 +165,7 @@ protected DeleteVersionsNode getDeepCopy() { * Else return MAX_VALUE. */ protected long prepare(Cell cell) { - boolean matchCq = - PrivateCellUtil.matchingQualifier(cell, lastCqArray, lastCqOffset, lastCqLength); - if (!matchCq) { + if (isColumnQualifierChanged(cell)) { // The last cell is family-level delete and this is not, or the cq is changed, // we should construct delColMap as a deep copy of delFamMap. delColMap.clear(); @@ -175,8 +173,7 @@ protected long prepare(Cell cell) { delColMap.put(e.getKey(), e.getValue().getDeepCopy()); } countCurrentCol = 0; - } - if (matchCq && !PrivateCellUtil.isDelete(lastCqType) && lastCqType == cell.getTypeByte() + } else if (!PrivateCellUtil.isDelete(lastCqType) && lastCqType == cell.getTypeByte() && lastCqTs == cell.getTimestamp()) { // Put with duplicate timestamp, ignore. return lastCqMvcc; @@ -190,6 +187,15 @@ protected long prepare(Cell cell) { return Long.MAX_VALUE; } + private boolean isColumnQualifierChanged(Cell cell) { + if (delColMap.isEmpty() + && (PrivateCellUtil.isDeleteColumns(cell) || PrivateCellUtil.isDeleteColumnVersion(cell))) { + // for null columnQualifier + return true; + } + return !PrivateCellUtil.matchingQualifier(cell, lastCqArray, lastCqOffset, lastCqLength); + } + // DeleteTracker @Override public void add(Cell cell) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java index 582f6c9542e0..3e21452c4e7c 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java @@ -19,7 +19,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; - +import static org.junit.Assert.assertTrue; import java.io.IOException; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtil; @@ -358,4 +358,14 @@ public void testRawScanAndMajorCompaction() throws IOException { } } + @Test + public void testNullColumnQualifier() throws IOException { + try (Table t = createTable()) { + Delete del = new Delete(ROW); + del.addColumn(FAMILY, null); + t.delete(del); + Result r = t.get(new Get(ROW)); //NPE + assertTrue(r.isEmpty()); + } + } } From 47cef724bea5b4018b138ae5c972997dc40e5f5a Mon Sep 17 00:00:00 2001 From: "jeongmin.kim" Date: Wed, 30 Mar 2022 09:40:06 +0900 Subject: [PATCH 2/3] HBASE-26901 delete with null columnQualifier occurs NullPointerException when NewVersionBehavior is on --- .../NewVersionBehaviorTracker.java | 110 +++++++++--------- 1 file changed, 52 insertions(+), 58 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java index 6bbb1ebb205b..1a75cfb8d764 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java @@ -1,19 +1,16 @@ /** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at *

* http://www.apache.org/licenses/LICENSE-2.0 *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. */ package org.apache.hadoop.hbase.regionserver.querymatcher; @@ -36,8 +33,8 @@ import org.apache.hadoop.hbase.regionserver.querymatcher.ScanQueryMatcher.MatchCode; /** - * A tracker both implementing ColumnTracker and DeleteTracker, used for mvcc-sensitive scanning. - * We should make sure in one QueryMatcher the ColumnTracker and DeleteTracker is the same instance. + * A tracker both implementing ColumnTracker and DeleteTracker, used for mvcc-sensitive scanning. We + * should make sure in one QueryMatcher the ColumnTracker and DeleteTracker is the same instance. */ @InterfaceAudience.Private public class NewVersionBehaviorTracker implements ColumnTracker, DeleteTracker { @@ -71,13 +68,12 @@ public class NewVersionBehaviorTracker implements ColumnTracker, DeleteTracker { /** * Note maxVersion and minVersion must set according to cf's conf, not user's scan parameter. - * - * @param columns columns specified user in query - * @param comparartor the cell comparator - * @param minVersion The minimum number of versions to keep(used when TTL is set). - * @param maxVersion The maximum number of versions in CF's conf + * @param columns columns specified user in query + * @param comparartor the cell comparator + * @param minVersion The minimum number of versions to keep(used when TTL is set). + * @param maxVersion The maximum number of versions in CF's conf * @param resultMaxVersions maximum versions to return per column, which may be different from - * maxVersion + * maxVersion * @param oldestUnexpiredTS the oldest timestamp we are interested in, based on TTL */ public NewVersionBehaviorTracker(NavigableSet columns, CellComparator comparartor, @@ -103,8 +99,8 @@ public void beforeShipped() throws IOException { } /** - * A data structure which contains infos we need that happens before this node's mvcc and - * after the previous node's mvcc. A node means there is a version deletion at the mvcc and ts. + * A data structure which contains infos we need that happens before this node's mvcc and after + * the previous node's mvcc. A node means there is a version deletion at the mvcc and ts. */ protected class DeleteVersionsNode { public long ts; @@ -158,11 +154,10 @@ protected DeleteVersionsNode getDeepCopy() { } /** - * Reset the map if it is different with the last Cell. - * Save the cq array/offset/length for next Cell. - * - * @return If this put has duplicate ts with last cell, return the mvcc of last cell. - * Else return MAX_VALUE. + * Reset the map if it is different with the last Cell. Save the cq array/offset/length for next + * Cell. + * @return If this put has duplicate ts with last cell, return the mvcc of last cell. Else return + * MAX_VALUE. */ protected long prepare(Cell cell) { if (isColumnQualifierChanged(cell)) { @@ -175,9 +170,9 @@ protected long prepare(Cell cell) { countCurrentCol = 0; } else if (!PrivateCellUtil.isDelete(lastCqType) && lastCqType == cell.getTypeByte() && lastCqTs == cell.getTimestamp()) { - // Put with duplicate timestamp, ignore. - return lastCqMvcc; - } + // Put with duplicate timestamp, ignore. + return lastCqMvcc; + } lastCqArray = cell.getQualifierArray(); lastCqOffset = cell.getQualifierOffset(); lastCqLength = cell.getQualifierLength(); @@ -188,7 +183,7 @@ protected long prepare(Cell cell) { } private boolean isColumnQualifierChanged(Cell cell) { - if (delColMap.isEmpty() + if (delColMap.isEmpty() && lastCqArray == null && cell.getQualifierLength() == 0 && (PrivateCellUtil.isDeleteColumns(cell) || PrivateCellUtil.isDeleteColumnVersion(cell))) { // for null columnQualifier return true; @@ -202,25 +197,25 @@ public void add(Cell cell) { prepare(cell); byte type = cell.getTypeByte(); switch (Type.codeToType(type)) { - // By the order of seen. We put null cq at first. - case DeleteFamily: // Delete all versions of all columns of the specified family - delFamMap.put(cell.getSequenceId(), + // By the order of seen. We put null cq at first. + case DeleteFamily: // Delete all versions of all columns of the specified family + delFamMap.put(cell.getSequenceId(), new DeleteVersionsNode(cell.getTimestamp(), cell.getSequenceId())); - break; - case DeleteFamilyVersion: // Delete all columns of the specified family and specified version - delFamMap.ceilingEntry(cell.getSequenceId()).getValue().addVersionDelete(cell); - break; - - // These two kinds of markers are mix with Puts. - case DeleteColumn: // Delete all versions of the specified column - delColMap.put(cell.getSequenceId(), + break; + case DeleteFamilyVersion: // Delete all columns of the specified family and specified version + delFamMap.ceilingEntry(cell.getSequenceId()).getValue().addVersionDelete(cell); + break; + + // These two kinds of markers are mix with Puts. + case DeleteColumn: // Delete all versions of the specified column + delColMap.put(cell.getSequenceId(), new DeleteVersionsNode(cell.getTimestamp(), cell.getSequenceId())); - break; - case Delete: // Delete the specified version of the specified column. - delColMap.ceilingEntry(cell.getSequenceId()).getValue().addVersionDelete(cell); - break; - default: - throw new AssertionError("Unknown delete marker type for " + cell); + break; + case Delete: // Delete the specified version of the specified column. + delColMap.ceilingEntry(cell.getSequenceId()).getValue().addVersionDelete(cell); + break; + default: + throw new AssertionError("Unknown delete marker type for " + cell); } } @@ -244,9 +239,8 @@ public DeleteResult isDeleted(Cell cell) { deleteMvcc = tail.first(); } } - SortedMap> subMap = - node.mvccCountingMap - .subMap(cell.getSequenceId(), true, Math.min(duplicateMvcc, deleteMvcc), true); + SortedMap> subMap = node.mvccCountingMap.subMap(cell.getSequenceId(), + true, Math.min(duplicateMvcc, deleteMvcc), true); for (Map.Entry> seg : subMap.entrySet()) { if (seg.getValue().size() >= maxVersions) { return DeleteResult.VERSION_MASKED; @@ -278,17 +272,17 @@ public void update() { // ignore } - //ColumnTracker + // ColumnTracker @Override public MatchCode checkColumn(Cell cell, byte type) throws IOException { if (columns == null) { - return MatchCode.INCLUDE; + return MatchCode.INCLUDE; } while (!done()) { - int c = CellUtil.compareQualifiers(cell, - columns[columnIndex], 0, columns[columnIndex].length); + int c = + CellUtil.compareQualifiers(cell, columns[columnIndex], 0, columns[columnIndex].length); if (c < 0) { return MatchCode.SEEK_NEXT_COL; } @@ -305,8 +299,8 @@ public MatchCode checkColumn(Cell cell, byte type) throws IOException { } @Override - public MatchCode checkVersions(Cell cell, long timestamp, byte type, - boolean ignoreCount) throws IOException { + public MatchCode checkVersions(Cell cell, long timestamp, byte type, boolean ignoreCount) + throws IOException { assert !PrivateCellUtil.isDelete(type); // We drop old version in #isDeleted, so here we won't SKIP because of versioning. But we should // consider TTL. @@ -350,7 +344,7 @@ public void reset() { resetInternal(); } - protected void resetInternal(){ + protected void resetInternal() { delFamMap.put(Long.MAX_VALUE, new DeleteVersionsNode()); } From 1f95ebb779a625bc1a425c3af8768e2307fd50b6 Mon Sep 17 00:00:00 2001 From: "jeongmin.kim" Date: Wed, 30 Mar 2022 09:53:24 +0900 Subject: [PATCH 3/3] HBASE-26901 delete with null columnQualifier occurs NullPointerException when NewVersionBehavior is on --- .../querymatcher/NewVersionBehaviorTracker.java | 16 +++++++++++----- .../TestNewVersionBehaviorFromClientSide.java | 11 +++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java index 3c9a541d0b38..d62e2aa1f5b6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NewVersionBehaviorTracker.java @@ -165,9 +165,7 @@ protected DeleteVersionsNode getDeepCopy() { * Else return MAX_VALUE. */ protected long prepare(Cell cell) { - boolean matchCq = - PrivateCellUtil.matchingQualifier(cell, lastCqArray, lastCqOffset, lastCqLength); - if (!matchCq) { + if (isColumnQualifierChanged(cell)) { // The last cell is family-level delete and this is not, or the cq is changed, // we should construct delColMap as a deep copy of delFamMap. delColMap.clear(); @@ -175,8 +173,7 @@ protected long prepare(Cell cell) { delColMap.put(e.getKey(), e.getValue().getDeepCopy()); } countCurrentCol = 0; - } - if (matchCq && !PrivateCellUtil.isDelete(lastCqType) && lastCqType == cell.getTypeByte() + } else if (!PrivateCellUtil.isDelete(lastCqType) && lastCqType == cell.getTypeByte() && lastCqTs == cell.getTimestamp()) { // Put with duplicate timestamp, ignore. return lastCqMvcc; @@ -190,6 +187,15 @@ protected long prepare(Cell cell) { return Long.MAX_VALUE; } + private boolean isColumnQualifierChanged(Cell cell) { + if (delColMap.isEmpty() && lastCqArray == null && cell.getQualifierLength() == 0 + && (PrivateCellUtil.isDeleteColumns(cell) || PrivateCellUtil.isDeleteColumnVersion(cell))) { + // for null columnQualifier + return true; + } + return !PrivateCellUtil.matchingQualifier(cell, lastCqArray, lastCqOffset, lastCqLength); + } + // DeleteTracker @Override public void add(Cell cell) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java index 582f6c9542e0..fd1663c01c1d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestNewVersionBehaviorFromClientSide.java @@ -19,6 +19,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import java.io.IOException; import org.apache.hadoop.hbase.HBaseClassTestRule; @@ -358,4 +359,14 @@ public void testRawScanAndMajorCompaction() throws IOException { } } + @Test + public void testNullColumnQualifier() throws IOException { + try (Table t = createTable()) { + Delete del = new Delete(ROW); + del.addColumn(FAMILY, null); + t.delete(del); + Result r = t.get(new Get(ROW)); //NPE + assertTrue(r.isEmpty()); + } + } }