HBase
  1. HBase
  2. HBASE-5041

Major compaction on non existing table does not throw error

    Details

    • Hadoop Flags:
      Reviewed

      Description

      Following will not complain even if fubar does not exist

      echo "major_compact 'fubar'" | $HBASE_HOME/bin/hbase shell
      

      The downside for this defect is that major compaction may be skipped due to
      a typo by Ops.

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2617 (See https://builds.apache.org/job/HBase-TRUNK/2617/)
        HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet)

        tedyu :
        Files :

        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2617 (See https://builds.apache.org/job/HBase-TRUNK/2617/ ) HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet) tedyu : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92-security #65 (See https://builds.apache.org/job/HBase-0.92-security/65/)
        HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet)

        tedyu :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Show
        Hudson added a comment - Integrated in HBase-0.92-security #65 (See https://builds.apache.org/job/HBase-0.92-security/65/ ) HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet) tedyu : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #66 (See https://builds.apache.org/job/HBase-TRUNK-security/66/)
        HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet)

        tedyu :
        Files :

        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #66 (See https://builds.apache.org/job/HBase-TRUNK-security/66/ ) HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet) tedyu : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92 #228 (See https://builds.apache.org/job/HBase-0.92/228/)
        HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet)

        tedyu :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Show
        Hudson added a comment - Integrated in HBase-0.92 #228 (See https://builds.apache.org/job/HBase-0.92/228/ ) HBASE-5041 Major compaction on non existing table does not throw error (Shrijeet) tedyu : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Hide
        Ted Yu added a comment -

        Integrated to 0.90, 0.92 and TRUNK

        Thanks for the patch Shrijeet

        Thanks for the review Stack

        Show
        Ted Yu added a comment - Integrated to 0.90, 0.92 and TRUNK Thanks for the patch Shrijeet Thanks for the review Stack
        Hide
        Ted Yu added a comment -

        Integration in the morning is great.
        Thanks, Stack.

        Show
        Ted Yu added a comment - Integration in the morning is great. Thanks, Stack.
        Hide
        stack added a comment -

        +1 on patch for trunk and for 0.92. Would suggest on commit that Shrijeet's nice 'commit message' be included (I can do the commit in morning since I'm the one insisting, so Ram has a chance to take a looksee)

        Show
        stack added a comment - +1 on patch for trunk and for 0.92. Would suggest on commit that Shrijeet's nice 'commit message' be included (I can do the commit in morning since I'm the one insisting, so Ram has a chance to take a looksee)
        Hide
        Ted Yu added a comment -

        @Stack, @Ram:
        Do you want to take another look ?

        This JIRA is marked against 0.92.0
        So I would integrate so that it is in 0.92 RC3.

        Show
        Ted Yu added a comment - @Stack, @Ram: Do you want to take another look ? This JIRA is marked against 0.92.0 So I would integrate so that it is in 0.92 RC3.
        Hide
        Jean-Daniel Cryans added a comment -

        +1

        Show
        Jean-Daniel Cryans added a comment - +1
        Hide
        Ted Yu added a comment -

        Here is the OS I used:

        Linux A 2.6.38-11-generic #48-Ubuntu SMP Fri Jul 29 19:02:55 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux
        

        I will integrate the patches tomorrow if there is no objection.

        Show
        Ted Yu added a comment - Here is the OS I used: Linux A 2.6.38-11- generic #48-Ubuntu SMP Fri Jul 29 19:02:55 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux I will integrate the patches tomorrow if there is no objection.
        Hide
        Shrijeet Paliwal added a comment -
         mvn clean compile test -Dtest=TestReplication
        

        Above passes without error for branch 0.90 in my dev machine.

        -Shrijeet

        Show
        Shrijeet Paliwal added a comment - mvn clean compile test -Dtest=TestReplication Above passes without error for branch 0.90 in my dev machine. -Shrijeet
        Hide
        Ted Yu added a comment -

        TestReplication failed in test suite run.
        When I ran it separately, I got:

        Failed tests: 
          queueFailover(org.apache.hadoop.hbase.replication.TestReplication)
        

        Looking at the patch for 0.90, it doesn't seem to be related to replication.

        @J-D:
        Your review/comment on the patch would be helpful.

        Thanks

        Show
        Ted Yu added a comment - TestReplication failed in test suite run. When I ran it separately, I got: Failed tests: queueFailover(org.apache.hadoop.hbase.replication.TestReplication) Looking at the patch for 0.90, it doesn't seem to be related to replication. @J-D: Your review/comment on the patch would be helpful. Thanks
        Hide
        Ted Yu added a comment -

        @Shrijeet:
        I am running patch for 0.90 through test suite.

        Will integrate if there is no test failure.

        Show
        Ted Yu added a comment - @Shrijeet: I am running patch for 0.90 through test suite. Will integrate if there is no test failure.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12509436/0003-HBASE-5041-Throw-error-if-table-does-not-exist.0.90.patch
        against trunk revision .

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/664//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/12509436/0003-HBASE-5041-Throw-error-if-table-does-not-exist.0.90.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/664//console This message is automatically generated.
        Hide
        Shrijeet Paliwal added a comment -

        Uploading patch for 0.90 branch. Sorry for the delay Ted. Noticed your comment late that you wanted to merge it yesterday itself.

        Show
        Shrijeet Paliwal added a comment - Uploading patch for 0.90 branch. Sorry for the delay Ted. Noticed your comment late that you wanted to merge it yesterday itself.
        Hide
        Ted Yu added a comment -

        @Shrijeet:
        I got the following when I tried to apply your patch on 0.90:

        5 out of 7 hunks FAILED -- saving rejects to file src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java.rej
        patching file src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Hunk #1 FAILED at 90.
        1 out of 1 hunk FAILED -- saving rejects to file src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java.rej
        

        Do you mind preparing another patch for 0.90 branch so that I can apply them on the same day ?

        Good job.

        Show
        Ted Yu added a comment - @Shrijeet: I got the following when I tried to apply your patch on 0.90: 5 out of 7 hunks FAILED -- saving rejects to file src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java.rej patching file src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java Hunk #1 FAILED at 90. 1 out of 1 hunk FAILED -- saving rejects to file src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java.rej Do you mind preparing another patch for 0.90 branch so that I can apply them on the same day ? Good job.
        Hide
        Ted Yu added a comment -

        Latest Hadoop QA report contains known test failures.
        Latest patch should be good to go.

        Show
        Ted Yu added a comment - Latest Hadoop QA report contains known test failures. Latest patch should be good to go.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12509373/0002-HBASE-5041-Throw-error-if-table-does-not-exist.patch
        against trunk revision .

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

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

        -1 javadoc. The javadoc tool appears to have generated -151 warning messages.

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

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

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

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/660//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/660//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/660//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/12509373/0002-HBASE-5041-Throw-error-if-table-does-not-exist.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 77 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/660//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/660//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/660//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        @Shrijeet:
        Please grant license to ASF when you attach patches.

        Thanks

        Show
        Ted Yu added a comment - @Shrijeet: Please grant license to ASF when you attach patches. Thanks
        Hide
        Ted Yu added a comment -

        @Shrijeet:
        Hadoop QA would run test suite on a new attachment.
        You can attach the same patch again.

        Show
        Ted Yu added a comment - @Shrijeet: Hadoop QA would run test suite on a new attachment. You can attach the same patch again.
        Hide
        Shrijeet Paliwal added a comment -

        Will resubmit

        Show
        Shrijeet Paliwal added a comment - Will resubmit
        Hide
        Shrijeet Paliwal added a comment -

        @Ted,
        For me it is failing randomly on some, but not the one you mentioned. In current test run (which is going on as I type this), it has failed on org.apache.hadoop.hbase.TestZooKeeper and org.apache.hadoop.hbase.replication.TestMasterReplication . It passed the one you mentioned. Interestingly hadoop QA reported some other failures (not the ones you or myself saw).

        OK. How do I resubmit the patch for Hadoop QA? I dont see the retrigger click on jenkins.

        Show
        Shrijeet Paliwal added a comment - @Ted, For me it is failing randomly on some, but not the one you mentioned. In current test run (which is going on as I type this), it has failed on org.apache.hadoop.hbase.TestZooKeeper and org.apache.hadoop.hbase.replication.TestMasterReplication . It passed the one you mentioned. Interestingly hadoop QA reported some other failures (not the ones you or myself saw). OK. How do I resubmit the patch for Hadoop QA? I dont see the retrigger click on jenkins.
        Hide
        Ted Yu added a comment -

        I got the following error running TestSplitLogManager with patch v2:

        Failed tests:   testMultipleResubmits(org.apache.hadoop.hbase.master.TestSplitLogManager)
        

        @Shrijeet:
        Do you mind running patch v2 through Hadoop QA again ?

        Show
        Ted Yu added a comment - I got the following error running TestSplitLogManager with patch v2: Failed tests: testMultipleResubmits(org.apache.hadoop.hbase.master.TestSplitLogManager) @Shrijeet: Do you mind running patch v2 through Hadoop QA again ?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12509345/0002-HBASE-5041-Throw-error-if-table-does-not-exist.patch
        against trunk revision .

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

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

        -1 javadoc. The javadoc tool appears to have generated -151 warning messages.

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

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

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

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/655//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/655//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/655//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/12509345/0002-HBASE-5041-Throw-error-if-table-does-not-exist.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 77 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/655//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/655//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/655//console This message is automatically generated.
        Hide
        Shrijeet Paliwal added a comment -

        Attaching second path based on Stack's and Ted's comment.

        Show
        Shrijeet Paliwal added a comment - Attaching second path based on Stack's and Ted's comment.
        Hide
        Shrijeet Paliwal added a comment -

        I will update this Jira with new Patch post holidays.

        Show
        Shrijeet Paliwal added a comment - I will update this Jira with new Patch post holidays.
        Hide
        Ted Yu added a comment -

        Sorry for getting back to this so late.
        I think we can keep the signature for split, flush and compact methods.

        @Shrijeet:
        Please attach new patch so that Hadoop QA can test it.

        Thanks

        Show
        Ted Yu added a comment - Sorry for getting back to this so late. I think we can keep the signature for split, flush and compact methods. @Shrijeet: Please attach new patch so that Hadoop QA can test it. Thanks
        Hide
        Shrijeet Paliwal added a comment -

        Also could one of you suggest if it makes sense to be explicitly declare throwing of TNFE in method signature of split, flush and compact ? Although we dont need it since TNFE is subclass on IOE, but ... what is best practice?

        Show
        Shrijeet Paliwal added a comment - Also could one of you suggest if it makes sense to be explicitly declare throwing of TNFE in method signature of split, flush and compact ? Although we dont need it since TNFE is subclass on IOE, but ... what is best practice?
        Hide
        Ted Yu added a comment -

        Shrijeet is using this method which was already in HBaseAdmin.java:

          private void cleanupCatalogTracker(final CatalogTracker ct) {
            ct.stop();
          }
        
        Show
        Ted Yu added a comment - Shrijeet is using this method which was already in HBaseAdmin.java: private void cleanupCatalogTracker( final CatalogTracker ct) { ct.stop(); }
        Hide
        Shrijeet Paliwal added a comment -

        @Stack

        I think patch is doing right thing. Its changing the contract for isRegionName but this is a private method and you are tightening what was a sloppy contract previous; it looks too like all instances of isRegionName can benefit from this tightening (is this your though Shrijeet?).

        Yes that is the idea.

        You might make a method that returns a String tablename for a table you know exists (else it throws the TNFE).

        Makes sense, will do.

        We are creating a new CatalogTracker instance. No one seems to be shutting it down? Is that a prob?

        Did not understand this one Stack. cleanupCatalogTracker called in finally will stop the CatalogTracker, no?

        Show
        Shrijeet Paliwal added a comment - @Stack I think patch is doing right thing. Its changing the contract for isRegionName but this is a private method and you are tightening what was a sloppy contract previous; it looks too like all instances of isRegionName can benefit from this tightening (is this your though Shrijeet?). Yes that is the idea. You might make a method that returns a String tablename for a table you know exists (else it throws the TNFE). Makes sense, will do. We are creating a new CatalogTracker instance. No one seems to be shutting it down? Is that a prob? Did not understand this one Stack. cleanupCatalogTracker called in finally will stop the CatalogTracker, no?
        Hide
        stack added a comment -

        I think patch is doing right thing. Its changing the contract for isRegionName but this is a private method and you are tightening what was a sloppy contract previous; it looks too like all instances of isRegionName can benefit from this tightening (is this your though Shrijeet?).

        Here's a few comments on the patch:

        This bit of code is repeated three time:

        +        final String tableName = Bytes.toString(tableNameOrRegionName);
        +        if (!MetaReader.tableExists(ct, tableName)) {
        +          throw new TableNotFoundException(tableName);
        +        }
        

        ... which isn't the end of the world but if you are going to cut a new patch..... you might make a method that returns a String tablename for a table you know exists (else it throws the TNFE).

        Then, given who the author of this patch is, I wonder about the below:

        +    CatalogTracker ct = getCatalogTracker();
        +    try {
        +      return (MetaReader.getRegion(ct, tableNameOrRegionName) != null);
        +    } finally {
        +      cleanupCatalogTracker(ct);
        +    }
        

        We are creating a new CatalogTracker instance. No one seems to be shutting it down? Is that a prob?

        Otherwise, patch is good by me.

        Show
        stack added a comment - I think patch is doing right thing. Its changing the contract for isRegionName but this is a private method and you are tightening what was a sloppy contract previous; it looks too like all instances of isRegionName can benefit from this tightening (is this your though Shrijeet?). Here's a few comments on the patch: This bit of code is repeated three time: + final String tableName = Bytes.toString(tableNameOrRegionName); + if (!MetaReader.tableExists(ct, tableName)) { + throw new TableNotFoundException(tableName); + } ... which isn't the end of the world but if you are going to cut a new patch..... you might make a method that returns a String tablename for a table you know exists (else it throws the TNFE). Then, given who the author of this patch is, I wonder about the below: + CatalogTracker ct = getCatalogTracker(); + try { + return (MetaReader.getRegion(ct, tableNameOrRegionName) != null ); + } finally { + cleanupCatalogTracker(ct); + } We are creating a new CatalogTracker instance. No one seems to be shutting it down? Is that a prob? Otherwise, patch is good by me.
        Hide
        Shrijeet Paliwal added a comment -

        @Ted, will add a unit test and upload a new one on top of trunk.

        @Ram, thanks for commenting. Do you mean to say isRegionName should throw an exception? I wanted to keep the semantic same as before - it tells weather the name argument 'appears' to be a region name or not. When MetaReader.getRegion returns null we know one thing for sure, it is not a region. Determining if its a valid table is left to caller, depending on need.

        Did you mean something else?

        Show
        Shrijeet Paliwal added a comment - @Ted, will add a unit test and upload a new one on top of trunk. @Ram, thanks for commenting. Do you mean to say isRegionName should throw an exception? I wanted to keep the semantic same as before - it tells weather the name argument 'appears' to be a region name or not. When MetaReader.getRegion returns null we know one thing for sure, it is not a region. Determining if its a valid table is left to caller, depending on need. Did you mean something else?
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Shrijeet

        if (isRegionName(tableNameOrRegionName)) {
                Pair<HRegionInfo, HServerAddress> pair =
                  MetaReader.getRegion(ct, tableNameOrRegionName);
                if (pair == null || pair.getSecond() == null) {
                  LOG.info("No server in .META. for " +
                    Bytes.toStringBinary(tableNameOrRegionName) + "; pair=" + pair);
        

        The MetaReader.getRegion should return null pair if a wrong table name is provided. Can't we throw exception there? May be am missing something.

        Show
        ramkrishna.s.vasudevan added a comment - @Shrijeet if (isRegionName(tableNameOrRegionName)) { Pair<HRegionInfo, HServerAddress> pair = MetaReader.getRegion(ct, tableNameOrRegionName); if (pair == null || pair.getSecond() == null ) { LOG.info( "No server in .META. for " + Bytes.toStringBinary(tableNameOrRegionName) + "; pair=" + pair); The MetaReader.getRegion should return null pair if a wrong table name is provided. Can't we throw exception there? May be am missing something.
        Hide
        Ted Yu added a comment -

        Can you add a new test verifying that compacting table 'fubar' would result in TableNotFoundException ?

        Show
        Ted Yu added a comment - Can you add a new test verifying that compacting table 'fubar' would result in TableNotFoundException ?
        Hide
        Ted Yu added a comment -

        Patch (for 0.90) looks good.

        Please attach patch for TRUNK so that Hadoop QA can do its job.

        Show
        Ted Yu added a comment - Patch (for 0.90) looks good. Please attach patch for TRUNK so that Hadoop QA can do its job.
        Hide
        Shrijeet Paliwal added a comment -

        Attaching first patch.

        Show
        Shrijeet Paliwal added a comment - Attaching first patch.
        Hide
        Shrijeet Paliwal added a comment -

        Will do.

        Show
        Shrijeet Paliwal added a comment - Will do.
        Hide
        Ted Yu added a comment -

        That should work. We should check validity of region names.
        Can you come up with a patch ?

        Show
        Ted Yu added a comment - That should work. We should check validity of region names. Can you come up with a patch ?
        Hide
        Shrijeet Paliwal added a comment -

        One possibility is make call to MetaReader.getRegion for the name and return true/false based on not-null/null value.

        Show
        Shrijeet Paliwal added a comment - One possibility is make call to MetaReader.getRegion for the name and return true/false based on not-null/null value.
        Hide
        Shrijeet Paliwal added a comment -

        Our logic to check if the name is a regionname or tablename is designed to be as follows:
        tl;dr: If it is not an existing table, its should be a region.

         /**
           * @param tableNameOrRegionName Name of a table or name of a region.
           * @return True if <code>tableNameOrRegionName</code> is *possibly* a region
           * name else false if a verified tablename (we call {@link #tableExists(byte[])};
           * else we throw an exception.
           * @throws IOException 
           */
          private boolean isRegionName(final byte [] tableNameOrRegionName)
          throws IOException {
            if (tableNameOrRegionName == null) {
              throw new IllegalArgumentException("Pass a table name or region name");
            }
            return !tableExists(tableNameOrRegionName);
          }
        

        My plan was to modify majorCompact function's else block to check if the table exist and throw TableNotFoundException if it does not.
        But because of name logic one will never reach 'else' part and a compaction request will be registered assuming it must be a region.

        Show
        Shrijeet Paliwal added a comment - Our logic to check if the name is a regionname or tablename is designed to be as follows: tl;dr: If it is not an existing table, its should be a region. /** * @param tableNameOrRegionName Name of a table or name of a region. * @return True if <code>tableNameOrRegionName</code> is *possibly* a region * name else false if a verified tablename (we call {@link #tableExists(byte[])}; * else we throw an exception. * @throws IOException */ private boolean isRegionName(final byte [] tableNameOrRegionName) throws IOException { if (tableNameOrRegionName == null) { throw new IllegalArgumentException("Pass a table name or region name"); } return !tableExists(tableNameOrRegionName); } My plan was to modify majorCompact function's else block to check if the table exist and throw TableNotFoundException if it does not. But because of name logic one will never reach 'else' part and a compaction request will be registered assuming it must be a region.

          People

          • Assignee:
            Shrijeet Paliwal
            Reporter:
            Shrijeet Paliwal
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development