From a5dda318b8eb0c1c907b28e94ea32fa6874646f2 Mon Sep 17 00:00:00 2001 From: openinx Date: Sat, 21 Oct 2017 21:38:22 +0800 Subject: [PATCH] HBASE-19057 Fix other code review comments about FilterList Improvement --- dev-support/docker/Dockerfile | 29 ++++++++----------- .../org/apache/hadoop/hbase/filter/FilterList.java | 19 +++++++++++-- .../apache/hadoop/hbase/filter/FilterListBase.java | 16 +++++++++-- .../hadoop/hbase/filter/FilterListWithAND.java | 11 ++------ .../hadoop/hbase/filter/FilterListWithOR.java | 33 +++++++++++----------- .../apache/hadoop/hbase/filter/TestFilterList.java | 4 ++- 6 files changed, 64 insertions(+), 48 deletions(-) diff --git a/dev-support/docker/Dockerfile b/dev-support/docker/Dockerfile index 62c6030..c23c70d 100644 --- a/dev-support/docker/Dockerfile +++ b/dev-support/docker/Dockerfile @@ -65,18 +65,18 @@ RUN apt-get -q update && apt-get -q install --no-install-recommends -y \ zlib1g-dev ####### -# Oracle Java +# OpenJDK 8 ####### RUN echo "dot_style = mega" > "/root/.wgetrc" RUN echo "quiet = on" >> "/root/.wgetrc" RUN apt-get -q update && apt-get -q install --no-install-recommends -y software-properties-common -RUN add-apt-repository -y ppa:webupd8team/java - -# Auto-accept the Oracle JDK license -RUN echo oracle-java8-installer shared/accepted-oracle-license-v1-1 select true | sudo /usr/bin/debconf-set-selections -RUN apt-get -q update && apt-get -q install --no-install-recommends -y oracle-java8-installer +RUN add-apt-repository -y ppa:openjdk-r/ppa +RUN apt-get -q update +RUN apt-get -q install --no-install-recommends -y openjdk-8-jdk +RUN update-alternatives --config java +RUN update-alternatives --config javac #### # Apps that require Java @@ -131,23 +131,16 @@ RUN pip install python-dateutil # Install Ruby 2, based on Yetus 0.4.0 dockerfile ### RUN echo 'gem: --no-rdoc --no-ri' >> /root/.gemrc -RUN apt-get -q install -y ruby2.0 -# -# on trusty, the above installs ruby2.0 and ruby (1.9.3) exes -# but update-alternatives is broken, so we need to do some work -# to make 2.0 actually the default without the system flipping out -# -# See https://bugs.launchpad.net/ubuntu/+source/ruby2.0/+bug/1310292 -# -RUN dpkg-divert --add --rename --divert /usr/bin/ruby.divert /usr/bin/ruby -RUN dpkg-divert --add --rename --divert /usr/bin/gem.divert /usr/bin/gemrc -RUN update-alternatives --install /usr/bin/ruby ruby /usr/bin/ruby2.0 1 -RUN update-alternatives --install /usr/bin/gem gem /usr/bin/gem2.0 1 +RUN apt-add-repository ppa:brightbox/ruby-ng +RUN apt-get -q update +RUN apt-get -q install --no-install-recommends -y ruby2.2 ruby-switch +RUN ruby-switch --set ruby2.2 #### # Install rubocop ### +RUN gem install rake RUN gem install rubocop #### 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 79f3e78..0f3057f 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 @@ -164,8 +164,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 + * currentTransformedCell. This parameter indicates the initial value of transformed cell before + * this filter operation.
+ * 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 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 06da6be..1d30fd69 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 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..a478694 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 @@ -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; } } @@ -262,9 +260,6 @@ public class FilterListWithAND extends FilterListBase { } Cell maxHint = null; for (Filter filter : seekHintFilter) { - if (filter.filterAllRemaining()) { - continue; - } Cell curKeyHint = filter.getNextCellHint(currentCell); if (maxHint == null) { maxHint = curKeyHint; 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..f467cf3 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,8 +182,10 @@ public class FilterListWithOR extends FilterListBase { if (isInReturnCodes(rc, ReturnCode.INCLUDE)) { return ReturnCode.INCLUDE; } - if (isInReturnCodes(rc, ReturnCode.INCLUDE_AND_NEXT_COL, - ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW)) { + 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; } if (isInReturnCodes(rc, ReturnCode.SKIP, ReturnCode.SEEK_NEXT_USING_HINT)) { @@ -254,7 +257,8 @@ public class FilterListWithOR extends FilterListBase { Filter filter = filters.get(i); Cell prevCell = this.prevCellList.get(i); - if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, i)) { + ReturnCode prevRC = this.prevFilterRCList.get(i); + if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, prevRC)) { everyFilterReturnHint = false; continue; } @@ -295,11 +299,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 +331,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 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 = false; } } @@ -373,9 +375,6 @@ public class FilterListWithOR extends FilterListBase { Cell minKeyHint = null; // If any condition can pass, we need to keep the min hint for (int i = 0, n = filters.size(); i < n; i++) { - if (filters.get(i).filterAllRemaining()) { - continue; - } Cell curKeyHint = filters.get(i).getNextCellHint(currentCell); if (curKeyHint == null) { // If we ever don't have a hint and this is must-pass-one, then no hint 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 c55094d..e4c6638 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 @@ -326,7 +326,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"); @@ -698,6 +698,7 @@ public class TestFilterList { filter.filterKeyValue(kv3); assertFalse(mockFilter.didCellPassToTheFilter); + filter.reset(); mockFilter.didCellPassToTheFilter = false; filter.filterKeyValue(kv4); assertTrue(mockFilter.didCellPassToTheFilter); @@ -715,6 +716,7 @@ public class TestFilterList { filter.filterKeyValue(kv3); assertFalse(mockFilter.didCellPassToTheFilter); + filter.reset(); mockFilter.didCellPassToTheFilter = false; filter.filterKeyValue(kv4); assertTrue(mockFilter.didCellPassToTheFilter); -- 2.7.4