You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by zh...@apache.org on 2017/10/25 12:37:41 UTC
[20/23] hbase git commit: HBASE-19057 Fix other code review comments
about FilterList improvement
HBASE-19057 Fix other code review comments about FilterList improvement
Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/705b3fa9
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/705b3fa9
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/705b3fa9
Branch: refs/heads/HBASE-18410
Commit: 705b3fa98c97806c7eba63617a99f62d829400d1
Parents: fcaf71d
Author: huzheng <op...@gmail.com>
Authored: Tue Oct 24 15:30:55 2017 +0800
Committer: zhangduo <zh...@apache.org>
Committed: Wed Oct 25 20:36:19 2017 +0800
----------------------------------------------------------------------
.../apache/hadoop/hbase/filter/FilterList.java | 24 ++++++++++-----
.../hadoop/hbase/filter/FilterListBase.java | 16 ++++++++--
.../hadoop/hbase/filter/FilterListWithAND.java | 12 ++++----
.../hadoop/hbase/filter/FilterListWithOR.java | 31 +++++++++++---------
.../hadoop/hbase/filter/TestFilterList.java | 8 +++--
5 files changed, 59 insertions(+), 32 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
index e87f1b3..d4242ae 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
@@ -21,17 +21,12 @@ package org.apache.hadoop.hbase.filter;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.Collection;
import java.util.Collections;
import java.util.List;
import org.apache.hadoop.hbase.Cell;
-import org.apache.hadoop.hbase.CellComparatorImpl;
-import org.apache.hadoop.hbase.CellUtil;
-import org.apache.hadoop.hbase.KeyValueUtil;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.exceptions.DeserializationException;
-import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.shaded.com.google.protobuf.InvalidProtocolBufferException;
import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
@@ -170,8 +165,23 @@ final public class FilterList extends FilterBase {
return filterListBase.transformCell(c);
}
- ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformedCell) throws IOException {
- return this.filterListBase.internalFilterKeyValue(c, currentTransformedCell);
+ /**
+ * Internal implementation of {@link #filterKeyValue(Cell)}. Compared to the
+ * {@link #filterKeyValue(Cell)} method, this method accepts an additional parameter named
+ * transformedCell. This parameter indicates the initial value of transformed cell before this
+ * filter operation. <br/>
+ * For FilterList, we can consider a filter list as a node in a tree. sub-filters of the filter
+ * list are children of the relative node. The logic of transforming cell of a filter list, well,
+ * we can consider it as the process of post-order tree traverse. For a node , Before we traverse
+ * the current child, we should set the traverse result (transformed cell) of previous node(s) as
+ * the initial value. so the additional currentTransformedCell parameter is needed (HBASE-18879).
+ * @param c The cell in question.
+ * @param transformedCell The transformed cell of previous filter(s)
+ * @return ReturnCode of this filter operation.
+ * @throws IOException
+ */
+ ReturnCode internalFilterKeyValue(Cell c, Cell transformedCell) throws IOException {
+ return this.filterListBase.internalFilterKeyValue(c, transformedCell);
}
@Override
http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
index 60b0dc1..f92d2e7 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
@@ -107,8 +107,20 @@ public abstract class FilterListBase extends FilterBase {
return cell;
}
- abstract ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformedCell)
- throws IOException;
+ /**
+ * Internal implementation of {@link #filterKeyValue(Cell)}
+ * @param c The cell in question.
+ * @param transformedCell The transformed cell of previous filter(s)
+ * @return ReturnCode of this filter operation.
+ * @throws IOException
+ * @see org.apache.hadoop.hbase.filter.FilterList#internalFilterKeyValue(Cell, Cell)
+ */
+ abstract ReturnCode internalFilterKeyValue(Cell c, Cell transformedCell) throws IOException;
+
+ @Override
+ public ReturnCode filterKeyValue(Cell c) throws IOException {
+ return internalFilterKeyValue(c, c);
+ }
/**
* Filters that never filter by modifying the returned List of Cells can inherit this
http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
index 4909dfd..9217ff9 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
@@ -148,12 +148,12 @@ public class FilterListWithAND extends FilterListBase {
}
@Override
- ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformedCell) throws IOException {
+ ReturnCode internalFilterKeyValue(Cell c, Cell transformedCell) throws IOException {
if (isEmpty()) {
return ReturnCode.INCLUDE;
}
ReturnCode rc = ReturnCode.INCLUDE;
- Cell transformed = currentTransformedCell;
+ Cell transformed = transformedCell;
this.referenceCell = c;
this.seekHintFilter.clear();
for (int i = 0, n = filters.size(); i < n; i++) {
@@ -186,11 +186,6 @@ public class FilterListWithAND extends FilterListBase {
}
@Override
- public ReturnCode filterKeyValue(Cell c) throws IOException {
- return internalFilterKeyValue(c, c);
- }
-
- @Override
public void reset() throws IOException {
for (int i = 0, n = filters.size(); i < n; i++) {
filters.get(i).reset();
@@ -222,6 +217,9 @@ public class FilterListWithAND extends FilterListBase {
for (int i = 0, n = filters.size(); i < n; i++) {
Filter filter = filters.get(i);
if (filter.filterAllRemaining() || filter.filterRowKey(firstRowCell)) {
+ // Can't just return true here, because there are some filters (such as PrefixFilter) which
+ // will catch the row changed event by filterRowKey(). If we return early here, those
+ // filters will have no chance to update their row state.
retVal = true;
}
}
http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
index 31e2a55..e51915b 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
@@ -81,9 +81,8 @@ public class FilterListWithOR extends FilterListBase {
* to the filter, if row mismatch or row match but column family mismatch. (HBASE-18368)
* @see org.apache.hadoop.hbase.filter.Filter.ReturnCode
*/
- private boolean shouldPassCurrentCellToFilter(Cell prevCell, Cell currentCell, int filterIdx)
- throws IOException {
- ReturnCode prevCode = this.prevFilterRCList.get(filterIdx);
+ private boolean shouldPassCurrentCellToFilter(Cell prevCell, Cell currentCell,
+ ReturnCode prevCode) throws IOException {
if (prevCell == null || prevCode == null) {
return true;
}
@@ -96,11 +95,13 @@ public class FilterListWithOR extends FilterListBase {
return nextHintCell == null || this.compareCell(currentCell, nextHintCell) >= 0;
case NEXT_COL:
case INCLUDE_AND_NEXT_COL:
- return !CellUtil.matchingRowColumn(prevCell, currentCell);
+ // Once row changed, reset() will clear prevCells, so we need not to compare their rows
+ // because rows are the same here.
+ return !CellUtil.matchingColumn(prevCell, currentCell);
case NEXT_ROW:
case INCLUDE_AND_SEEK_NEXT_ROW:
- return !CellUtil.matchingRows(prevCell, currentCell)
- || !CellUtil.matchingFamily(prevCell, currentCell);
+ // As described above, rows are definitely the same, so we only compare the family.
+ return !CellUtil.matchingFamily(prevCell, currentCell);
default:
throw new IllegalStateException("Received code is not valid.");
}
@@ -181,6 +182,9 @@ public class FilterListWithOR extends FilterListBase {
if (isInReturnCodes(rc, ReturnCode.INCLUDE)) {
return ReturnCode.INCLUDE;
}
+ if (isInReturnCodes(rc, ReturnCode.NEXT_COL, ReturnCode.NEXT_ROW)) {
+ return ReturnCode.NEXT_COL;
+ }
if (isInReturnCodes(rc, ReturnCode.INCLUDE_AND_NEXT_COL,
ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW)) {
return ReturnCode.INCLUDE_AND_NEXT_COL;
@@ -242,19 +246,20 @@ public class FilterListWithOR extends FilterListBase {
}
@Override
- ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformCell) throws IOException {
+ ReturnCode internalFilterKeyValue(Cell c, Cell transformCell) throws IOException {
if (isEmpty()) {
return ReturnCode.INCLUDE;
}
ReturnCode rc = null;
boolean everyFilterReturnHint = true;
- Cell transformed = currentTransformCell;
+ Cell transformed = transformCell;
this.referenceCell = c;
for (int i = 0, n = filters.size(); i < n; i++) {
Filter filter = filters.get(i);
Cell prevCell = this.prevCellList.get(i);
- if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, i)) {
+ ReturnCode prevCode = this.prevFilterRCList.get(i);
+ if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, prevCode)) {
everyFilterReturnHint = false;
continue;
}
@@ -295,11 +300,6 @@ public class FilterListWithOR extends FilterListBase {
}
@Override
- public ReturnCode filterKeyValue(Cell c) throws IOException {
- return internalFilterKeyValue(c, c);
- }
-
- @Override
public void reset() throws IOException {
for (int i = 0, n = filters.size(); i < n; i++) {
filters.get(i).reset();
@@ -332,6 +332,9 @@ public class FilterListWithOR extends FilterListBase {
for (int i = 0, n = filters.size(); i < n; i++) {
Filter filter = filters.get(i);
if (!filter.filterAllRemaining() && !filter.filterRowKey(firstRowCell)) {
+ // Can't just return false here, because there are some filters (such as PrefixFilter) which
+ // will catch the row changed event by filterRowKey(). If we return early here, those
+ // filters will have no chance to update their row state.
retVal = false;
}
}
http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
index d420b02..8c83cf6 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
@@ -330,7 +330,7 @@ public class TestFilterList {
* @throws Exception
*/
@Test
- public void testFilterListWithInclusiveStopFilteMustPassOne() throws Exception {
+ public void testFilterListWithInclusiveStopFilterMustPassOne() throws Exception {
byte[] r1 = Bytes.toBytes("Row1");
byte[] r11 = Bytes.toBytes("Row11");
byte[] r2 = Bytes.toBytes("Row2");
@@ -351,11 +351,13 @@ public class TestFilterList {
public AlwaysNextColFilter() {
super();
}
+
@Override
public ReturnCode filterKeyValue(Cell v) {
return ReturnCode.NEXT_COL;
}
- public static AlwaysNextColFilter parseFrom(final byte [] pbBytes)
+
+ public static AlwaysNextColFilter parseFrom(final byte[] pbBytes)
throws DeserializationException {
return new AlwaysNextColFilter();
}
@@ -701,6 +703,7 @@ public class TestFilterList {
filter.filterKeyValue(kv3);
assertFalse(mockFilter.didCellPassToTheFilter);
+ filter.reset();
mockFilter.didCellPassToTheFilter = false;
filter.filterKeyValue(kv4);
assertTrue(mockFilter.didCellPassToTheFilter);
@@ -718,6 +721,7 @@ public class TestFilterList {
filter.filterKeyValue(kv3);
assertFalse(mockFilter.didCellPassToTheFilter);
+ filter.reset();
mockFilter.didCellPassToTheFilter = false;
filter.filterKeyValue(kv4);
assertTrue(mockFilter.didCellPassToTheFilter);