HBase
  1. HBase
  2. HBASE-10965

Automate detection of presence of Filter#filterRow()

    Details

    • Type: Task Task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Filters
    • Labels:
      None

      Description

      There is potential inconsistency between the return value of Filter#hasFilterRow() and presence of Filter#filterRow().

      Filters may override Filter#filterRow() while leaving return value of Filter#hasFilterRow() being false (inherited from FilterBase).

      Downside to purely depending on hasFilterRow() telling us whether custom filter overrides filterRow(List) or filterRow() is that the check below may be rendered ineffective:

                if (nextKv == KV_LIMIT) {
                  if (this.filter != null && filter.hasFilterRow()) {
                    throw new IncompatibleFilterException(
                      "Filter whose hasFilterRow() returns true is incompatible with scan with limit!");
                  }
      

      When user forgets to override hasFilterRow(), the above check becomes not useful.

      Another limitation is that we cannot optimize FilterList#filterRow() through short circuit when FilterList#hasFilterRow() turns false.
      See https://issues.apache.org/jira/browse/HBASE-11093?focusedCommentId=13985149&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13985149

      This JIRA aims to remove the inconsistency by automatically detecting the presence of overridden Filter#filterRow(). For FilterBase-derived classes, if filterRow() is implemented and not inherited from FilterBase, it is equivalent to having hasFilterRow() return true.

      With precise detection of presence of Filter#filterRow(), the following code from HRegion is no longer needed while backward compatibility is kept.

            return filter != null && (!filter.hasFilterRow())
                && filter.filterRow();
      
      1. 10965-v1.txt
        4 kB
        Ted Yu
      2. 10965-v2.txt
        4 kB
        Ted Yu
      3. 10965-v3.txt
        5 kB
        Ted Yu
      4. 10965-v4.txt
        5 kB
        Ted Yu
      5. 10965-v6.txt
        10 kB
        Ted Yu
      6. 10965-v7.txt
        13 kB
        Ted Yu

        Issue Links

          Activity

          Hide
          Ted Yu added a comment -

          Patch v1 uses reflection to detect overridden Filter#filterRow().

          I can put isMethodImplemented() in a utility class once the approach passes review.

          *Filter* tests pass.

          Show
          Ted Yu added a comment - Patch v1 uses reflection to detect overridden Filter#filterRow(). I can put isMethodImplemented() in a utility class once the approach passes review. *Filter* tests pass.
          Hide
          Ted Yu added a comment -

          For Phoenix, I did the following:

          apply patch to 0.98 branch
          mvn clean package install -DskipTests
          in Phoenix workspace, change hbase-hadoop2.version to 0.98.2-SNAPSHOT
          Use command 'nohup ~/apache-maven-3.1.1/bin/mvn clean install -Dhadoop.profile=2 > h2.out &' to run unit tests and they passed.

          Show
          Ted Yu added a comment - For Phoenix, I did the following: apply patch to 0.98 branch mvn clean package install -DskipTests in Phoenix workspace, change hbase-hadoop2.version to 0.98.2-SNAPSHOT Use command 'nohup ~/apache-maven-3.1.1/bin/mvn clean install -Dhadoop.profile=2 > h2.out &' to run unit tests and they passed.
          Hide
          Ted Yu added a comment -

          TestMultiParallel fails on QA bot.
          But it doesn't use any filter and it passes locally on Mac.

          Show
          Ted Yu added a comment - TestMultiParallel fails on QA bot. But it doesn't use any filter and it passes locally on Mac.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12639884/10965-v1.txt
          against trunk revision .
          ATTACHMENT ID: 12639884

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch 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.client.TestMultiParallel

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//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/12639884/10965-v1.txt against trunk revision . ATTACHMENT ID: 12639884 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch 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.client.TestMultiParallel Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9263//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch v2 adds javadoc for isFilterMethodImplemented().

          Show
          Ted Yu added a comment - Patch v2 adds javadoc for isFilterMethodImplemented().
          Hide
          Anoop Sam John added a comment -

          We need similar change here also.
          HRegion#nextInternal(List<Cell> results, int limit)

          if (this.filter != null && filter.hasFilterRow()) {
            throw new IncompatibleFilterException(
                "Filter whose hasFilterRow() returns true is incompatible with scan with limit!");
          }
          

          Also we rely on this hasFilterRow() in other places too. There also some changes needed?

          Why is the change in FilterList? IMO in FilterList we have to decide based on individual filters within it. We can not return true directly.

          Show
          Anoop Sam John added a comment - We need similar change here also. HRegion#nextInternal(List<Cell> results, int limit) if ( this .filter != null && filter.hasFilterRow()) { throw new IncompatibleFilterException( "Filter whose hasFilterRow() returns true is incompatible with scan with limit!" ); } Also we rely on this hasFilterRow() in other places too. There also some changes needed? Why is the change in FilterList? IMO in FilterList we have to decide based on individual filters within it. We can not return true directly.
          Hide
          Ted Yu added a comment -

          The filter referenced in above code is of FilterWrapper where hasFilterRow() uses the return value from isFilterMethodImplemented().

          Also we rely on this hasFilterRow() in other places too.

          I searched all references to hasFilterRow() and made corresponding changes. Filter#hasFilterRow() can now be deprecated.

          Why is the change in FilterList

          This is to make hasFilterRow() consistent with the filterRow() method. Prior to this change, filterRow() iterates through filters and calls filterRow(). So the change is consistent with the description of Filter#hasFilterRow().

          Show
          Ted Yu added a comment - The filter referenced in above code is of FilterWrapper where hasFilterRow() uses the return value from isFilterMethodImplemented(). Also we rely on this hasFilterRow() in other places too. I searched all references to hasFilterRow() and made corresponding changes. Filter#hasFilterRow() can now be deprecated. Why is the change in FilterList This is to make hasFilterRow() consistent with the filterRow() method. Prior to this change, filterRow() iterates through filters and calls filterRow(). So the change is consistent with the description of Filter#hasFilterRow().
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12639910/10965-v2.txt
          against trunk revision .
          ATTACHMENT ID: 12639910

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch 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/9268//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//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/12639910/10965-v2.txt against trunk revision . ATTACHMENT ID: 12639910 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch 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/9268//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9268//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Will check this later so will get back on this tomorrow or monday.

          Show
          ramkrishna.s.vasudevan added a comment - Will check this later so will get back on this tomorrow or monday.
          Hide
          Ted Yu added a comment -

          Patch v3 addresses Anoop's comment on FilterList#hasFilterRow()

          Show
          Ted Yu added a comment - Patch v3 addresses Anoop's comment on FilterList#hasFilterRow()
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12639934/10965-v3.txt
          against trunk revision .
          ATTACHMENT ID: 12639934

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch 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/9271//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//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/12639934/10965-v3.txt against trunk revision . ATTACHMENT ID: 12639934 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch 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/9271//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9271//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch v4 is based on patch v3 and has the following additional change:

          -          if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE) || filterRow()) {
          +          if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE)) {
          

          The above is feasible because detection of presence of filterRow() is much more reliable now. We don't need to call filterRow twice.

          All *Filter* tests pass.

          Show
          Ted Yu added a comment - Patch v4 is based on patch v3 and has the following additional change: - if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE) || filterRow()) { + if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE)) { The above is feasible because detection of presence of filterRow() is much more reliable now. We don't need to call filterRow twice. All *Filter* tests pass.
          Hide
          stack added a comment -

          There is potential inconsistency between the return value of Filter#hasFilterRow() and presence of Filter#filterRow(). Filters may override Filter#filterRow() while leaving return value of Filter#hasFilterRow() being false (inherited from FilterBase).

          The particular filter implementation is broke then? You would change how we run filters to accommodate brokenly implemented filters?

          This JIRA aims to remove the inconsistency by automatically detecting the presence of overridden Filter#filterRow(). If filterRow() is implemented and not inherited from FilterBase, it is equivalent to having hasFilterRow() return true.

          Why do we need this? Who has this problem? Are folks implementing filters w/o reading the Interface (is it not clear on what needs overriding – if not, shouldn't we fix that?) or who is implementing filters w/o test that proves their filter "does the right thing?" Is the filter API broke? Should we fix that rather than add what looks like a band-aid, one that may 'surprise' implementors?

          Show
          stack added a comment - There is potential inconsistency between the return value of Filter#hasFilterRow() and presence of Filter#filterRow(). Filters may override Filter#filterRow() while leaving return value of Filter#hasFilterRow() being false (inherited from FilterBase). The particular filter implementation is broke then? You would change how we run filters to accommodate brokenly implemented filters? This JIRA aims to remove the inconsistency by automatically detecting the presence of overridden Filter#filterRow(). If filterRow() is implemented and not inherited from FilterBase, it is equivalent to having hasFilterRow() return true. Why do we need this? Who has this problem? Are folks implementing filters w/o reading the Interface (is it not clear on what needs overriding – if not, shouldn't we fix that?) or who is implementing filters w/o test that proves their filter "does the right thing?" Is the filter API broke? Should we fix that rather than add what looks like a band-aid, one that may 'surprise' implementors?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12639959/10965-v4.txt
          against trunk revision .
          ATTACHMENT ID: 12639959

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch 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/9272//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//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/12639959/10965-v4.txt against trunk revision . ATTACHMENT ID: 12639959 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch 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/9272//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9272//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment - - edited

          Is this right?

          -          if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE) || filterRow()) {
          +          if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE)) {
          

          And this? (why not continue to call hasFilterRow(), which we just fixed in the patch to do right thing, and more importantly cache the outcode)

          -    if (this.hasFilter() && this.filter.hasFilterRow()) {
          +    if (this.hasFilter() && FilterWrapper.isFilterMethodImplemented(this.filter, "filterRow")) {
          

          The last point raises a bigger discussion: If we autodetect this, it should be removed from the filter API completely and moved into the framework. I.e. there's no need for Filter to even have hasFilterRow() anymore.

          Lastly, it would certainly be nice to autodetect this. Is it worth the effort, though?

          Show
          Lars Hofhansl added a comment - - edited Is this right? - if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE) || filterRow()) { + if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE)) { And this? (why not continue to call hasFilterRow(), which we just fixed in the patch to do right thing, and more importantly cache the outcode) - if ( this .hasFilter() && this .filter.hasFilterRow()) { + if ( this .hasFilter() && FilterWrapper.isFilterMethodImplemented( this .filter, "filterRow" )) { The last point raises a bigger discussion: If we autodetect this, it should be removed from the filter API completely and moved into the framework. I.e. there's no need for Filter to even have hasFilterRow() anymore. Lastly, it would certainly be nice to autodetect this. Is it worth the effort, though?
          Hide
          Anoop Sam John added a comment -
          +    if (hasFilterRow == null) {
          +      for (Filter filter : filters) {
          +        if (FilterWrapper.isFilterMethodImplemented(filter, "filterRow")) {
          +          this.hasFilterRow = true;
          +          return true;
          +        }
                 }
          +      this.hasFilterRow = false;
               }
          

          This will be problematic When a FL contains another FL, the inner one will always say hasFilterRow as true.

          So here as per V4 patch we will auto detect presence of filterRow() overridden alone?

          FilterWrapper#hasFilterRow() just checks presence of filterRow() ? ! When Filter extending
          filterRowCells(List<Cell> kvs) or filterRow(List<KeyValue> kvs) hasFilter() have to return true (And we need fail in HRegion a scan with filter and batch being set)

          Show
          Anoop Sam John added a comment - + if (hasFilterRow == null ) { + for (Filter filter : filters) { + if (FilterWrapper.isFilterMethodImplemented(filter, "filterRow" )) { + this .hasFilterRow = true ; + return true ; + } } + this .hasFilterRow = false ; } This will be problematic When a FL contains another FL, the inner one will always say hasFilterRow as true. So here as per V4 patch we will auto detect presence of filterRow() overridden alone? FilterWrapper#hasFilterRow() just checks presence of filterRow() ? ! When Filter extending filterRowCells(List<Cell> kvs) or filterRow(List<KeyValue> kvs) hasFilter() have to return true (And we need fail in HRegion a scan with filter and batch being set)
          Hide
          Anoop Sam John added a comment -

          In initial time of 94 , I remember that Filter#hasFilterRow() was associated with Filter#filterRow(List<KV>)
          We used this boolean API in 2 places
          1. To check whether Scan is with a Filter, which is having row filtering, and with a batch value set. This is not correct way and we have to fail then
          2. The place before we call Filter#filterRow(List<KV>) in HRegion

          Later we made it like hasFilterRow() to return true when filterRow() is also implemented and so same is used in code where we call filterRow() also.

          Actually we need to use this boolean API only in case 1. When one uses a filter and a Scan batch wrongly, we will fail the scan op 1st step itself. So there is no need to check this boolean API value before calling the actual Filter methods. The impl in FilterBase do nothing and no problem in calling that I believe.

          So with this Jira, if there is consensus to remove even the hasFilterRow() method, we can simplify the things. Just in place where we check the Scan filter and batch to see whether we can proceed with the open scanner, just do this auto detect of method impls and act. In other places of HRegion scan flow, no need to rely on any of these boolean values of hasFilterRow() returns.

          Make some sense?

          When we auto detect presence of methods take care of the FL case. ( FL inside FL) which I was saying above.

          Show
          Anoop Sam John added a comment - In initial time of 94 , I remember that Filter#hasFilterRow() was associated with Filter#filterRow(List<KV>) We used this boolean API in 2 places 1. To check whether Scan is with a Filter, which is having row filtering, and with a batch value set. This is not correct way and we have to fail then 2. The place before we call Filter#filterRow(List<KV>) in HRegion Later we made it like hasFilterRow() to return true when filterRow() is also implemented and so same is used in code where we call filterRow() also. Actually we need to use this boolean API only in case 1. When one uses a filter and a Scan batch wrongly, we will fail the scan op 1st step itself. So there is no need to check this boolean API value before calling the actual Filter methods. The impl in FilterBase do nothing and no problem in calling that I believe. So with this Jira, if there is consensus to remove even the hasFilterRow() method, we can simplify the things. Just in place where we check the Scan filter and batch to see whether we can proceed with the open scanner, just do this auto detect of method impls and act. In other places of HRegion scan flow, no need to rely on any of these boolean values of hasFilterRow() returns. Make some sense? When we auto detect presence of methods take care of the FL case. ( FL inside FL) which I was saying above.
          Hide
          ramkrishna.s.vasudevan added a comment -

          This will be problematic When a FL contains another FL, the inner one will always say hasFilterRow as true.

          Good catch.
          We could deprecate hasFilterRow() if there is a consensus in removing it. Later can remove it.
          You mean to use the hasFilterrow() only in openScanner and not use in every next() calls? Ideally if the

          if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE) || filterRow()) {
          

          filterRow() is getting removed here then hasfilterRow() is called to check the stopRow alone. In that case moving it to the openScanner makes sense instead of doing the reflection way of using it.

          Show
          ramkrishna.s.vasudevan added a comment - This will be problematic When a FL contains another FL, the inner one will always say hasFilterRow as true. Good catch. We could deprecate hasFilterRow() if there is a consensus in removing it. Later can remove it. You mean to use the hasFilterrow() only in openScanner and not use in every next() calls? Ideally if the if ((isEmptyRow || ret == FilterWrapper.FilterRowRetCode.EXCLUDE) || filterRow()) { filterRow() is getting removed here then hasfilterRow() is called to check the stopRow alone. In that case moving it to the openScanner makes sense instead of doing the reflection way of using it.
          Hide
          Ted Yu added a comment -

          You would change how we run filters to accommodate brokenly implemented filters?

          Accommodating brokenly implemented filters is already in place. Please take a look at the following method in HRegion#RegionScannerImpl:

              private boolean filterRow() throws IOException {
                // when hasFilterRow returns true, filter.filterRow() will be called automatically inside
                // filterRowCells(List<Cell> kvs) so we skip that scenario here.
                return filter != null && (!filter.hasFilterRow())
                    && filter.filterRow();
              }
          

          When filter.hasFilterRow() returns false, filter.filterRow() is still called.

          Are folks implementing filters w/o reading the Interface

          Here is one example: phoenix-core/src/main/java/org/apache/phoenix/filter/MultiKeyValueComparisonFilter.java

              @Override
              public boolean filterRow() {
          

          But hasFilterRow() is not defined. There're other files in Phoenix which exhibit the same pattern.

          is it not clear on what needs overriding

          From hbase-client/src/main/java/org/apache/hadoop/hbase/filter/Filter.java (since 0.96):

            /**
             * Primarily used to check for conflicts with scans(such as scans that do not read a full row at a
             * time).
             *
             * @return True if this filter actively uses filterRow(List) or filterRow().
             */
            abstract public boolean hasFilterRow();
          

          The contract is clearly published but not all people follow it.

          If a custom filter overrides filterRow(List) or filterRow(), it is easy to use reflection to detect. Having hasFilterRow() method leaves room for inconsistency.

          If we can reach consensus on whether hasFilterRow() method should be deprecated first and then removed, I will address Lars' and Anoop's review comments in the next patch.

          Show
          Ted Yu added a comment - You would change how we run filters to accommodate brokenly implemented filters? Accommodating brokenly implemented filters is already in place. Please take a look at the following method in HRegion#RegionScannerImpl: private boolean filterRow() throws IOException { // when hasFilterRow returns true , filter.filterRow() will be called automatically inside // filterRowCells(List<Cell> kvs) so we skip that scenario here. return filter != null && (!filter.hasFilterRow()) && filter.filterRow(); } When filter.hasFilterRow() returns false, filter.filterRow() is still called. Are folks implementing filters w/o reading the Interface Here is one example: phoenix-core/src/main/java/org/apache/phoenix/filter/MultiKeyValueComparisonFilter.java @Override public boolean filterRow() { But hasFilterRow() is not defined. There're other files in Phoenix which exhibit the same pattern. is it not clear on what needs overriding From hbase-client/src/main/java/org/apache/hadoop/hbase/filter/Filter.java (since 0.96): /** * Primarily used to check for conflicts with scans(such as scans that do not read a full row at a * time). * * @ return True if this filter actively uses filterRow(List) or filterRow(). */ abstract public boolean hasFilterRow(); The contract is clearly published but not all people follow it. If a custom filter overrides filterRow(List) or filterRow(), it is easy to use reflection to detect. Having hasFilterRow() method leaves room for inconsistency. If we can reach consensus on whether hasFilterRow() method should be deprecated first and then removed, I will address Lars' and Anoop's review comments in the next patch.
          Hide
          Ted Yu added a comment -

          Another downside to purely depending on hasFilterRow() telling us whether custom filter overrides filterRow(List) or filterRow() is that the check below may be rendered ineffective:

                    if (nextKv == KV_LIMIT) {
                      if (this.filter != null && filter.hasFilterRow()) {
                        throw new IncompatibleFilterException(
                          "Filter whose hasFilterRow() returns true is incompatible with scan with limit!");
                      }
          

          When user forgets to override hasFilterRow(), the above check becomes not useful.

          Show
          Ted Yu added a comment - Another downside to purely depending on hasFilterRow() telling us whether custom filter overrides filterRow(List) or filterRow() is that the check below may be rendered ineffective: if (nextKv == KV_LIMIT) { if ( this .filter != null && filter.hasFilterRow()) { throw new IncompatibleFilterException( "Filter whose hasFilterRow() returns true is incompatible with scan with limit!" ); } When user forgets to override hasFilterRow(), the above check becomes not useful.
          Hide
          stack added a comment -

          Accommodating brokenly implemented filters is already in place.

          So the argument is that because we do something whack once, it is ok to continue along the whack path?

          Traditionally, compensations like these compounded makes the rig's operation fickle.

          And this example you cite seems categorically different from your patch in that it is the internals dictating how execution flows rather than compensation for what seems like a failure to implement the Interface as described (and that it calls filterRow though hasFilterRow is false is a bug).

          But hasFilterRow() is not defined. There're other files in Phoenix which exhibit the same pattern.

          Eh... file a bug with pheonix that they've failed to implement filters properly.

          The contract is clearly published but not all people follow it.

          The javadoc is not decisive. That could be tightened up. If folks don't read the contract, then it makes it tough. We should fail fast though rather than try make guess at what was intended.

          If this issue had examination of why we have these two tied methods at all I'd have some sympathy for the proposal but as is it looks to me to be making a bad situation worse.

          Show
          stack added a comment - Accommodating brokenly implemented filters is already in place. So the argument is that because we do something whack once, it is ok to continue along the whack path? Traditionally, compensations like these compounded makes the rig's operation fickle. And this example you cite seems categorically different from your patch in that it is the internals dictating how execution flows rather than compensation for what seems like a failure to implement the Interface as described (and that it calls filterRow though hasFilterRow is false is a bug). But hasFilterRow() is not defined. There're other files in Phoenix which exhibit the same pattern. Eh... file a bug with pheonix that they've failed to implement filters properly. The contract is clearly published but not all people follow it. The javadoc is not decisive. That could be tightened up. If folks don't read the contract, then it makes it tough. We should fail fast though rather than try make guess at what was intended. If this issue had examination of why we have these two tied methods at all I'd have some sympathy for the proposal but as is it looks to me to be making a bad situation worse.
          Hide
          Ted Yu added a comment -

          file a bug with pheonix that they've failed to implement filters properly.

          PHOENIX-910 was logged two weeks ago.

          Show
          Ted Yu added a comment - file a bug with pheonix that they've failed to implement filters properly. PHOENIX-910 was logged two weeks ago.
          Hide
          Ted Yu added a comment - - edited

          Toward the tail of HBASE-11093, Anoop makes a case that as long as the following code is present in HRegion, change in HBASE-11093 w.r.t. FilterList#filterRow() cannot be applied:

          private boolean filterRow() throws IOException {
            // when hasFilterRow returns true, filter.filterRow() will be called automatically inside
            // filterRowCells(List<Cell> kvs) so we skip that scenario here.
            return filter != null && (!filter.hasFilterRow())
          	  && filter.filterRow();
          }
          

          See https://issues.apache.org/jira/browse/HBASE-11093?focusedCommentId=13985149&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13985149

          I think goal of this JIRA can be achieved without removing hasFilterRow().
          New method for autodetecting presence of hasFilterRow() can be added to FilterWrapper. We can rely on this new method in place where hasFilterRow() is currently called in HRegion.
          Post 1.0 release, we can remove hasFilterRow().

          Show
          Ted Yu added a comment - - edited Toward the tail of HBASE-11093 , Anoop makes a case that as long as the following code is present in HRegion, change in HBASE-11093 w.r.t. FilterList#filterRow() cannot be applied: private boolean filterRow() throws IOException { // when hasFilterRow returns true , filter.filterRow() will be called automatically inside // filterRowCells(List<Cell> kvs) so we skip that scenario here. return filter != null && (!filter.hasFilterRow()) && filter.filterRow(); } See https://issues.apache.org/jira/browse/HBASE-11093?focusedCommentId=13985149&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13985149 I think goal of this JIRA can be achieved without removing hasFilterRow(). New method for autodetecting presence of hasFilterRow() can be added to FilterWrapper. We can rely on this new method in place where hasFilterRow() is currently called in HRegion. Post 1.0 release, we can remove hasFilterRow().
          Hide
          Ted Yu added a comment -

          Patch v6 distinguishes the user filters which directly override Filter class from those which extend FilterBase.

          FilterUtil class is added which contain two helper methods.

          All *Filter* tests pass.

          Show
          Ted Yu added a comment - Patch v6 distinguishes the user filters which directly override Filter class from those which extend FilterBase. FilterUtil class is added which contain two helper methods. All *Filter* tests pass.
          Hide
          Ted Yu added a comment -

          From https://builds.apache.org/job/PreCommit-HBASE-Build/9449/consoleFull :

          HBASE-10965 patch is being downloaded at Fri May  2 20:53:24 UTC 2014 from
          http://issues.apache.org/jira/secure/attachment/12643103/10965-v6.txt
          ...
          
          Results :
          
          Tests run: 0, Failures: 0, Errors: 0, Skipped: 0
          
          [INFO] 
          [INFO] --- maven-surefire-plugin:2.12-TRUNK-HBASE-2:test (secondPartTestsExecution) @ hbase-shell ---
          [INFO] Surefire report directory: /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/hbase-shell/target/surefire-reports
          [INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
          ...
          FATAL: hudson.remoting.RequestAbortedException: java.io.IOException: Unexpected termination of the channel
          hudson.remoting.RequestAbortedException: hudson.remoting.RequestAbortedException: java.io.IOException: Unexpected termination of the channel
          	at hudson.remoting.RequestAbortedException.wrapForRethrow(RequestAbortedException.java:41)
          	at hudson.remoting.RequestAbortedException.wrapForRethrow(RequestAbortedException.java:34)
          

          Basically hbase-server tests passed but the build got aborted.

          Show
          Ted Yu added a comment - From https://builds.apache.org/job/PreCommit-HBASE-Build/9449/consoleFull : HBASE-10965 patch is being downloaded at Fri May 2 20:53:24 UTC 2014 from http: //issues.apache.org/jira/secure/attachment/12643103/10965-v6.txt ... Results : Tests run: 0, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] --- maven-surefire-plugin:2.12-TRUNK-HBASE-2:test (secondPartTestsExecution) @ hbase-shell --- [INFO] Surefire report directory: /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/hbase-shell/target/surefire-reports [INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider ... FATAL: hudson.remoting.RequestAbortedException: java.io.IOException: Unexpected termination of the channel hudson.remoting.RequestAbortedException: hudson.remoting.RequestAbortedException: java.io.IOException: Unexpected termination of the channel at hudson.remoting.RequestAbortedException.wrapForRethrow(RequestAbortedException.java:41) at hudson.remoting.RequestAbortedException.wrapForRethrow(RequestAbortedException.java:34) Basically hbase-server tests passed but the build got aborted.
          Hide
          Ted Yu added a comment -

          Patch v7 adds test where a user filter extends Filter directly.

          Show
          Ted Yu added a comment - Patch v7 adds test where a user filter extends Filter directly.
          Hide
          Ted Yu added a comment -

          I ran test suite on Linux. There were 3 test failures:

          Failed tests:   testQuarantineMissingHFile(org.apache.hadoop.hbase.util.TestHBaseFsck): expected:<2> but was:<1>
          
          Tests in error:
            testFlushCommitsWithAbort(org.apache.hadoop.hbase.client.TestMultiParallel): test timed out after 300000 milliseconds
            testMasterRestartWhenTableInEnabling(org.apache.hadoop.hbase.master.TestAssignmentManager)
          

          The above tests don't use filter.
          They passed on re-run.

          Show
          Ted Yu added a comment - I ran test suite on Linux. There were 3 test failures: Failed tests: testQuarantineMissingHFile(org.apache.hadoop.hbase.util.TestHBaseFsck): expected:<2> but was:<1> Tests in error: testFlushCommitsWithAbort(org.apache.hadoop.hbase.client.TestMultiParallel): test timed out after 300000 milliseconds testMasterRestartWhenTableInEnabling(org.apache.hadoop.hbase.master.TestAssignmentManager) The above tests don't use filter. They passed on re-run.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12643213/10965-v7.txt
          against trunk revision .
          ATTACHMENT ID: 12643213

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

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

          +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 1.3.9) 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/9457//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//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/12643213/10965-v7.txt against trunk revision . ATTACHMENT ID: 12643213 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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 1.3.9) 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/9457//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9457//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          v7 looks like it should work in all cases and not cause performance a degradation.

          Is the complexity worth it here, though? The interface is easily described, we need to fix our internal filters and all implementors of Filter need to do the same.

          Show
          Lars Hofhansl added a comment - v7 looks like it should work in all cases and not cause performance a degradation. Is the complexity worth it here, though? The interface is easily described, we need to fix our internal filters and all implementors of Filter need to do the same.
          Hide
          Ted Yu added a comment -

          w.r.t. complexity, patch v7 creates FilterUtil class for utility methods and adds new unit test. Apart from those, the patch is small.

          all implementors of Filter need to do the same.

          There're many custom Filters in use today. It is hard to know which filters override filterRow() but don't override hasFilterRow() - without autodetection proposed in this JIRA.
          Hence the following code from HRegion cannot be safely removed without autodetection of hasFilterRow() :

                return filter != null && (!filter.hasFilterRow())
                    && filter.filterRow();
          

          One limitation is that we cannot optimize FilterList#filterRow() through short circuit when FilterList#hasFilterRow() turns false. HBASE-11093 is blocked due to this reason.

          Show
          Ted Yu added a comment - w.r.t. complexity, patch v7 creates FilterUtil class for utility methods and adds new unit test. Apart from those, the patch is small. all implementors of Filter need to do the same. There're many custom Filters in use today. It is hard to know which filters override filterRow() but don't override hasFilterRow() - without autodetection proposed in this JIRA. Hence the following code from HRegion cannot be safely removed without autodetection of hasFilterRow() : return filter != null && (!filter.hasFilterRow()) && filter.filterRow(); One limitation is that we cannot optimize FilterList#filterRow() through short circuit when FilterList#hasFilterRow() turns false. HBASE-11093 is blocked due to this reason.
          Hide
          ramkrishna.s.vasudevan added a comment -
          +      Class<? extends Object> declaringClass = m.getDeclaringClass();
          +      Class<? extends Object> superCls = declaringClass.getSuperclass();
          +      if (declaringClass.equals(clazz) && superCls.equals(filterCls)) {
          +        // filter class directly overrides Filter
          +        return filter.hasFilterRow();
          +      }
          

          If the filter is just implementing directly the 'Filter' class.
          'declaringClass.getSuperclass();' would return null? Am not sure. Can you verify this once? If so 'null' check may be needed.

          Show
          ramkrishna.s.vasudevan added a comment - + Class <? extends Object > declaringClass = m.getDeclaringClass(); + Class <? extends Object > superCls = declaringClass.getSuperclass(); + if (declaringClass.equals(clazz) && superCls.equals(filterCls)) { + // filter class directly overrides Filter + return filter.hasFilterRow(); + } If the filter is just implementing directly the 'Filter' class. 'declaringClass.getSuperclass();' would return null? Am not sure. Can you verify this once? If so 'null' check may be needed.
          Hide
          Lars Hofhansl added a comment -

          -0 on this change. Just seems to add complexity for little gain.
          If we could remove hasFilterRow completely from the interface (which we can do in 1.0, I think) that would be a different story.
          Now we have the worst of both worlds. Filter still has hasFilterRow() and on top of that we have the reflection stuff.

          Show
          Lars Hofhansl added a comment - -0 on this change. Just seems to add complexity for little gain. If we could remove hasFilterRow completely from the interface (which we can do in 1.0, I think) that would be a different story. Now we have the worst of both worlds. Filter still has hasFilterRow() and on top of that we have the reflection stuff.
          Hide
          Anoop Sam John added a comment -

          My concern also same. We have to rely on the boolean return value as well as we have auto detect. Code is more complex now. I initially thought we can remove hasFilterRow () But as long as we have Filter and FilterBase 2 abstract super classes we can not remove the api.

          Show
          Anoop Sam John added a comment - My concern also same. We have to rely on the boolean return value as well as we have auto detect. Code is more complex now. I initially thought we can remove hasFilterRow () But as long as we have Filter and FilterBase 2 abstract super classes we can not remove the api.
          Hide
          Ted Yu added a comment -

          In 0.94, Filter is an interface.
          This means user filters coming from 0.94 would always override FilterBase.

          Show
          Ted Yu added a comment - In 0.94, Filter is an interface. This means user filters coming from 0.94 would always override FilterBase.
          Hide
          Ted Yu added a comment -

          From Filter.java in 0.96 :

           * Interface for row and column filters directly applied within the regionserver.
          ...
           * When implementing your own filters, consider inheriting {@link FilterBase} to help
           * you reduce boilerplate.
          
          Show
          Ted Yu added a comment - From Filter.java in 0.96 : * Interface for row and column filters directly applied within the regionserver. ... * When implementing your own filters, consider inheriting {@link FilterBase} to help * you reduce boilerplate.

            People

            • Assignee:
              Ted Yu
              Reporter:
              Ted Yu
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:

                Development