You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by hyunsik <gi...@git.apache.org> on 2015/09/17 22:07:33 UTC

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

GitHub user hyunsik opened a pull request:

    https://github.com/apache/tajo/pull/768

    TAJO-1868: Allow TablespaceManager::get to return unregistered tables…

    …pace.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/hyunsik/tajo TAJO-1868

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/tajo/pull/768.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #768
    
----
commit af62c81faf66966305596fb6effb90c7e34f6094
Author: Hyunsik Choi <hy...@apache.org>
Date:   2015-09-17T19:56:12Z

    TAJO-1868: Allow TablespaceManager::get to return unregistered tablespace.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141881348
  
    Hi @jinossy and @hyunsik, both points look reasonable to me. 
    Even though we use FileTablespace as the default handler when the handler is not found, Hadoop's FileSystem implementation will emit an error of ```No FileSystem for scheme: [uri_scheme]```.
    In some sense, this error will be more useful for users than ```tablespace '/path/to/table' does not exist```.
    On the other hand, I'm also concerned with unexpected behaviours due to some other bugs if we don't handle exceptions about Tablespace by ourselves.
    
    What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141350668
  
    @hyunsik, it looks that there is still a problem.
    ```
    default> create external table customer (id int4) using text location 's3a://id:key@tajo-data-sa-east-1/tpch-1g/customer/customer.tbl';
    ERROR: internal error: Not found Tablespace handler for s3a://id:key@tajo-data-sa-east-1/tpch-1g/customer/customer.tbl
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141972054
  
    The case where someone develops custom file system rarely occurs in real field. That's too much.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/768#discussion_r39824725
  
    --- Diff: tajo-storage/tajo-storage-pgsql/src/test/java/org/apache/tajo/storage/pgsql/TestPgSQLQueryTests.java ---
    @@ -30,6 +30,8 @@
     import java.util.HashMap;
     import java.util.Map;
     
    +import static org.junit.Assert.assertTrue;
    --- End diff --
    
    Unused imports.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-142166876
  
    Ok. Let's consider it again when it comes to a problem.
    Anyway, how about changing ```CreateTableExecutor::getTablespaceHandler()``` to throw the ```UndefinedTablespaceHandlerException``` instead of ```UndefinedTablespaceException```? This is not related to this patch, but related to the previous one.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141858748
  
    @hyunsik 
    I recommend default handler to FileTablespace, if a handler not found. What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-142342006
  
    +1 the latest patch looks good to me. I left a trivial comment. If you agree, please reflect before commit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/768#discussion_r40108916
  
    --- Diff: tajo-common/src/main/java/org/apache/tajo/exception/ErrorMessages.java ---
    @@ -72,6 +72,7 @@
         ADD_MESSAGE(UNDEFINED_INDEX_FOR_TABLE, "index ''%s' does not exist", 1);
         ADD_MESSAGE(UNDEFINED_INDEX_FOR_COLUMNS, "index does not exist for '%s' columns of '%s' table", 2);
         ADD_MESSAGE(UNDEFINED_INDEX_NAME, "index name '%s' does not exist", 1);
    +    ADD_MESSAGE(UNDEFINED_TABLESPACE_HANDLER, "No tablespace handler for URI scheme '%s'", 1);
    --- End diff --
    
    IMHO, kind and plentiful (not detailed) error messages will make users happy. How about adding an warning message like ```Please check your table location uri scheme```?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141377453
  
    It may be because there is no entry for ``s3a`` in ``storage-default.json``. Probably, ``s3n`` may has the same problem. I'll add more entries for them.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/768#discussion_r40139286
  
    --- Diff: tajo-common/src/main/java/org/apache/tajo/exception/ErrorMessages.java ---
    @@ -72,6 +72,7 @@
         ADD_MESSAGE(UNDEFINED_INDEX_FOR_TABLE, "index ''%s' does not exist", 1);
         ADD_MESSAGE(UNDEFINED_INDEX_FOR_COLUMNS, "index does not exist for '%s' columns of '%s' table", 2);
         ADD_MESSAGE(UNDEFINED_INDEX_NAME, "index name '%s' does not exist", 1);
    +    ADD_MESSAGE(UNDEFINED_TABLESPACE_HANDLER, "No tablespace handler for URI scheme '%s'", 1);
    --- End diff --
    
    Almost all error messages of ours show just the fact. One example is ``invalid Session '%s'``, and another example is ``index name '%s' does not exist``. Users cannot know what they mean actually if they do not have knowledge about internal. This is because ``index name`` are likely to be generated automatically and many users do not know what is the session.
    
    Our case is the same. Its fact is that there is no tablespace handler to support the uri scheme. Your suggested error message ``Please check your table location uri scheme`` is actually based on some assumption that users may mistypo the uri. But, it may not be true in some cases. Also, this error message is inconsistent with others.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/tajo/pull/768


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141865538
  
    @jinossy In my opinion, that would be not good because all arbitrary schemes will be handled by FileTablespace. For example, assume that users can mistype the uri like ``hdfx://host:port``. If the default handler exists regardless of table uri, hdfx will be handled FileTablespace. It will cause unexpected and ugly errors.
    
    I think that the best way is to add all possible uri schemes to storage-default.json.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141892912
  
    @jihoonson The error message was changed as follows: ``Not found Tablespace handler for ${uri}``. It would look more reasonable error than that of before. If the problem is about error message, we can improve error messages. 
    
    In contrast, similar to your mention, if we use FileTablespace as  a default handler, it will make more unexpected cases because there are many code paths to handle the URI. It may make "out of control" against some error case.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-142223428
  
    ```CreateTableExecutor::getTablespaceHandler()``` still throws ```UndefinedTablespaceException```, but it is not necessary because methods called ```getTablespaceHandler()``` throws exceptions wrapped by ```TajoRuntimeException```. For the sake of consistency, please remove it. (But, I'm not sure that throwing exceptions wrapped by ```TajoRuntimeException``` is a good convention.)
    
    In addition, it would be great if you add any tests for ```UndefinedTablespaceHandlerException```.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-142196384
  
    Thank you for your comments. I've reflected your latest comment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-142238418
  
    Fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:

    https://github.com/apache/tajo/pull/768#issuecomment-141897129
  
    @hyunsik, 
    I understood. if someone add custom filesystem with hdfs, they must be add tablespace handler
    If we add a guide(link?) of adding tablespace in exception message, it would be better. I think


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/768#discussion_r39824860
  
    --- Diff: tajo-core-tests/src/test/java/org/apache/tajo/engine/query/TestHBaseTable.java ---
    @@ -177,195 +185,230 @@ public void testCreateRowFieldWithNonText() throws Exception {
     
       @Test
       public void testCreateExternalHBaseTable() throws Exception {
    -    HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table_not_purge"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col1"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col2"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col3"));
    -    testingCluster.getHBaseUtil().createTable(hTableDesc);
    -
    -    String sql = String.format(
    -        "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    -        "USING hbase WITH ('table'='external_hbase_table_not_purge', 'columns'=':key,col1:a,col2:,col3:b') " +
    -        "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    -    executeString(sql).close();
    +    Optional<Tablespace> existing = TablespaceManager.removeTablespaceForTest("cluster1");
    +    assertTrue(existing.isPresent());
     
    -    assertTableExists("external_hbase_mapped_table");
    +    try {
    +      HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table_not_purge"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col1"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col2"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col3"));
    +      testingCluster.getHBaseUtil().createTable(hTableDesc);
    +
    +      String sql = String.format(
    +          "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    +              "USING hbase WITH ('table'='external_hbase_table_not_purge', 'columns'=':key,col1:a,col2:,col3:b') " +
    +              "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    +      executeString(sql).close();
     
    -    executeString("DROP TABLE external_hbase_mapped_table").close();
    +      assertTableExists("external_hbase_mapped_table");
    +
    +      executeString("DROP TABLE external_hbase_mapped_table").close();
    +
    +      HBaseAdmin hAdmin = new HBaseAdmin(testingCluster.getHBaseUtil().getConf());
    +      try {
    +        assertTrue(hAdmin.tableExists("external_hbase_table_not_purge"));
    +        hAdmin.disableTable("external_hbase_table_not_purge");
    +        hAdmin.deleteTable("external_hbase_table_not_purge");
    +      } finally {
    +        hAdmin.close();
    +      }
     
    -    HBaseAdmin hAdmin =  new HBaseAdmin(testingCluster.getHBaseUtil().getConf());
    -    try {
    -      assertTrue(hAdmin.tableExists("external_hbase_table_not_purge"));
    -      hAdmin.disableTable("external_hbase_table_not_purge");
    -      hAdmin.deleteTable("external_hbase_table_not_purge");
         } finally {
    -      hAdmin.close();
    +      TablespaceManager.addTableSpaceForTest(existing.get());
         }
    +
       }
     
       @Test
       public void testSimpleSelectQuery() throws Exception {
    -    HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col1"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col2"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col3"));
    -    testingCluster.getHBaseUtil().createTable(hTableDesc);
    -
    -    String sql = String.format(
    -        "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    -        "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key,col1:a,col2:,col3:b') " +
    -        "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    -    executeString(sql).close();
    +    Optional<Tablespace> existing = TablespaceManager.removeTablespaceForTest("cluster1");
    +    assertTrue(existing.isPresent());
     
    -    assertTableExists("external_hbase_mapped_table");
    +    try {
    +      HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col1"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col2"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col3"));
    +      testingCluster.getHBaseUtil().createTable(hTableDesc);
    +
    +      String sql = String.format(
    +          "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    +              "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key,col1:a,col2:,col3:b') " +
    +              "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    +      executeString(sql).close();
     
    -    HBaseTablespace space = (HBaseTablespace) TablespaceManager.getByName("cluster1").get();
    -    HConnection hconn = space.getConnection();
    -    HTableInterface htable = hconn.getTable("external_hbase_table");
    +      assertTableExists("external_hbase_mapped_table");
     
    -    try {
    -      for (int i = 0; i < 100; i++) {
    -        Put put = new Put(String.valueOf(i).getBytes());
    -        put.add("col1".getBytes(), "a".getBytes(), ("a-" + i).getBytes());
    -        put.add("col1".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    -        put.add("col2".getBytes(), "k1".getBytes(), ("k1-" + i).getBytes());
    -        put.add("col2".getBytes(), "k2".getBytes(), ("k2-" + i).getBytes());
    -        put.add("col3".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    -        htable.put(put);
    -      }
    +      HConnection hconn = ((HBaseTablespace)existing.get()).getConnection();
    +      HTableInterface htable = hconn.getTable("external_hbase_table");
     
    -      ResultSet res = executeString("select * from external_hbase_mapped_table where rk > '20'");
    -      assertResultSet(res);
    -      cleanupQuery(res);
    +      try {
    +        for (int i = 0; i < 100; i++) {
    +          Put put = new Put(String.valueOf(i).getBytes());
    +          put.add("col1".getBytes(), "a".getBytes(), ("a-" + i).getBytes());
    +          put.add("col1".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    +          put.add("col2".getBytes(), "k1".getBytes(), ("k1-" + i).getBytes());
    +          put.add("col2".getBytes(), "k2".getBytes(), ("k2-" + i).getBytes());
    +          put.add("col3".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    +          htable.put(put);
    +        }
    +
    +        ResultSet res = executeString("select * from external_hbase_mapped_table where rk > '20'");
    +        assertResultSet(res);
    +        cleanupQuery(res);
    +      } finally {
    +        executeString("DROP TABLE external_hbase_mapped_table PURGE").close();
    +        htable.close();
    +      }
         } finally {
    -      executeString("DROP TABLE external_hbase_mapped_table PURGE").close();
    -      htable.close();
    +      TablespaceManager.addTableSpaceForTest(existing.get());
         }
       }
     
       @Test
       public void testBinaryMappedQuery() throws Exception {
    -    HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col1"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col2"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col3"));
    -    testingCluster.getHBaseUtil().createTable(hTableDesc);
    -
    -    String sql = String.format(
    -        "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk int8, col1 text, col2 text, col3 int4)\n " +
    -        "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key#b,col1:a,col2:,col3:b#b') " +
    -        "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    -    executeString(sql).close();
    -
    -    assertTableExists("external_hbase_mapped_table");
    -
    -    HBaseTablespace space = (HBaseTablespace) TablespaceManager.getByName("cluster1").get();
    -    HConnection hconn = space.getConnection();
    -    HTableInterface htable = hconn.getTable("external_hbase_table");
    +    //Optional<Tablespace> existing = TablespaceManager.removeTablespaceForTest("cluster1");
    --- End diff --
    
    Does this line need to be restored?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tajo pull request: TAJO-1868: Allow TablespaceManager::get to retu...

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/768#discussion_r39824864
  
    --- Diff: tajo-core-tests/src/test/java/org/apache/tajo/engine/query/TestHBaseTable.java ---
    @@ -177,195 +185,230 @@ public void testCreateRowFieldWithNonText() throws Exception {
     
       @Test
       public void testCreateExternalHBaseTable() throws Exception {
    -    HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table_not_purge"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col1"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col2"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col3"));
    -    testingCluster.getHBaseUtil().createTable(hTableDesc);
    -
    -    String sql = String.format(
    -        "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    -        "USING hbase WITH ('table'='external_hbase_table_not_purge', 'columns'=':key,col1:a,col2:,col3:b') " +
    -        "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    -    executeString(sql).close();
    +    Optional<Tablespace> existing = TablespaceManager.removeTablespaceForTest("cluster1");
    +    assertTrue(existing.isPresent());
     
    -    assertTableExists("external_hbase_mapped_table");
    +    try {
    +      HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table_not_purge"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col1"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col2"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col3"));
    +      testingCluster.getHBaseUtil().createTable(hTableDesc);
    +
    +      String sql = String.format(
    +          "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    +              "USING hbase WITH ('table'='external_hbase_table_not_purge', 'columns'=':key,col1:a,col2:,col3:b') " +
    +              "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    +      executeString(sql).close();
     
    -    executeString("DROP TABLE external_hbase_mapped_table").close();
    +      assertTableExists("external_hbase_mapped_table");
    +
    +      executeString("DROP TABLE external_hbase_mapped_table").close();
    +
    +      HBaseAdmin hAdmin = new HBaseAdmin(testingCluster.getHBaseUtil().getConf());
    +      try {
    +        assertTrue(hAdmin.tableExists("external_hbase_table_not_purge"));
    +        hAdmin.disableTable("external_hbase_table_not_purge");
    +        hAdmin.deleteTable("external_hbase_table_not_purge");
    +      } finally {
    +        hAdmin.close();
    +      }
     
    -    HBaseAdmin hAdmin =  new HBaseAdmin(testingCluster.getHBaseUtil().getConf());
    -    try {
    -      assertTrue(hAdmin.tableExists("external_hbase_table_not_purge"));
    -      hAdmin.disableTable("external_hbase_table_not_purge");
    -      hAdmin.deleteTable("external_hbase_table_not_purge");
         } finally {
    -      hAdmin.close();
    +      TablespaceManager.addTableSpaceForTest(existing.get());
         }
    +
       }
     
       @Test
       public void testSimpleSelectQuery() throws Exception {
    -    HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col1"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col2"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col3"));
    -    testingCluster.getHBaseUtil().createTable(hTableDesc);
    -
    -    String sql = String.format(
    -        "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    -        "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key,col1:a,col2:,col3:b') " +
    -        "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    -    executeString(sql).close();
    +    Optional<Tablespace> existing = TablespaceManager.removeTablespaceForTest("cluster1");
    +    assertTrue(existing.isPresent());
     
    -    assertTableExists("external_hbase_mapped_table");
    +    try {
    +      HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col1"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col2"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col3"));
    +      testingCluster.getHBaseUtil().createTable(hTableDesc);
    +
    +      String sql = String.format(
    +          "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk text, col1 text, col2 text, col3 text) " +
    +              "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key,col1:a,col2:,col3:b') " +
    +              "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    +      executeString(sql).close();
     
    -    HBaseTablespace space = (HBaseTablespace) TablespaceManager.getByName("cluster1").get();
    -    HConnection hconn = space.getConnection();
    -    HTableInterface htable = hconn.getTable("external_hbase_table");
    +      assertTableExists("external_hbase_mapped_table");
     
    -    try {
    -      for (int i = 0; i < 100; i++) {
    -        Put put = new Put(String.valueOf(i).getBytes());
    -        put.add("col1".getBytes(), "a".getBytes(), ("a-" + i).getBytes());
    -        put.add("col1".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    -        put.add("col2".getBytes(), "k1".getBytes(), ("k1-" + i).getBytes());
    -        put.add("col2".getBytes(), "k2".getBytes(), ("k2-" + i).getBytes());
    -        put.add("col3".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    -        htable.put(put);
    -      }
    +      HConnection hconn = ((HBaseTablespace)existing.get()).getConnection();
    +      HTableInterface htable = hconn.getTable("external_hbase_table");
     
    -      ResultSet res = executeString("select * from external_hbase_mapped_table where rk > '20'");
    -      assertResultSet(res);
    -      cleanupQuery(res);
    +      try {
    +        for (int i = 0; i < 100; i++) {
    +          Put put = new Put(String.valueOf(i).getBytes());
    +          put.add("col1".getBytes(), "a".getBytes(), ("a-" + i).getBytes());
    +          put.add("col1".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    +          put.add("col2".getBytes(), "k1".getBytes(), ("k1-" + i).getBytes());
    +          put.add("col2".getBytes(), "k2".getBytes(), ("k2-" + i).getBytes());
    +          put.add("col3".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    +          htable.put(put);
    +        }
    +
    +        ResultSet res = executeString("select * from external_hbase_mapped_table where rk > '20'");
    +        assertResultSet(res);
    +        cleanupQuery(res);
    +      } finally {
    +        executeString("DROP TABLE external_hbase_mapped_table PURGE").close();
    +        htable.close();
    +      }
         } finally {
    -      executeString("DROP TABLE external_hbase_mapped_table PURGE").close();
    -      htable.close();
    +      TablespaceManager.addTableSpaceForTest(existing.get());
         }
       }
     
       @Test
       public void testBinaryMappedQuery() throws Exception {
    -    HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col1"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col2"));
    -    hTableDesc.addFamily(new HColumnDescriptor("col3"));
    -    testingCluster.getHBaseUtil().createTable(hTableDesc);
    -
    -    String sql = String.format(
    -        "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk int8, col1 text, col2 text, col3 int4)\n " +
    -        "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key#b,col1:a,col2:,col3:b#b') " +
    -        "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    -    executeString(sql).close();
    -
    -    assertTableExists("external_hbase_mapped_table");
    -
    -    HBaseTablespace space = (HBaseTablespace) TablespaceManager.getByName("cluster1").get();
    -    HConnection hconn = space.getConnection();
    -    HTableInterface htable = hconn.getTable("external_hbase_table");
    +    //Optional<Tablespace> existing = TablespaceManager.removeTablespaceForTest("cluster1");
    +    //assertTrue(existing.isPresent());
     
         try {
    -      for (int i = 0; i < 100; i++) {
    -        Put put = new Put(Bytes.toBytes((long) i));
    -        put.add("col1".getBytes(), "a".getBytes(), ("a-" + i).getBytes());
    -        put.add("col1".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    -        put.add("col2".getBytes(), "k1".getBytes(), ("k1-" + i).getBytes());
    -        put.add("col2".getBytes(), "k2".getBytes(), ("k2-" + i).getBytes());
    -        put.add("col3".getBytes(), "b".getBytes(), Bytes.toBytes(i));
    -        htable.put(put);
    -      }
    +      HTableDescriptor hTableDesc = new HTableDescriptor(TableName.valueOf("external_hbase_table"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col1"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col2"));
    +      hTableDesc.addFamily(new HColumnDescriptor("col3"));
    +      testingCluster.getHBaseUtil().createTable(hTableDesc);
    +
    +      String sql = String.format(
    +          "CREATE EXTERNAL TABLE external_hbase_mapped_table (rk int8, col1 text, col2 text, col3 int4)\n " +
    +              "USING hbase WITH ('table'='external_hbase_table', 'columns'=':key#b,col1:a,col2:,col3:b#b') " +
    +              "LOCATION '%s/external_hbase_table'", tableSpaceUri);
    +      executeString(sql).close();
     
    -      ResultSet res = executeString("select * from external_hbase_mapped_table where rk > 20");
    -      assertResultSet(res);
    -      res.close();
    +      assertTableExists("external_hbase_mapped_table");
    +
    +      HBaseTablespace space = (HBaseTablespace) TablespaceManager.getByName("cluster1").get();
    +      HConnection hconn = space.getConnection();
    +      HTableInterface htable = hconn.getTable("external_hbase_table");
    +
    +      try {
    +        for (int i = 0; i < 100; i++) {
    +          Put put = new Put(Bytes.toBytes((long) i));
    +          put.add("col1".getBytes(), "a".getBytes(), ("a-" + i).getBytes());
    +          put.add("col1".getBytes(), "b".getBytes(), ("b-" + i).getBytes());
    +          put.add("col2".getBytes(), "k1".getBytes(), ("k1-" + i).getBytes());
    +          put.add("col2".getBytes(), "k2".getBytes(), ("k2-" + i).getBytes());
    +          put.add("col3".getBytes(), "b".getBytes(), Bytes.toBytes(i));
    +          htable.put(put);
    +        }
     
    -      //Projection
    -      res = executeString("select col3, col2, rk from external_hbase_mapped_table where rk > 95");
    +        ResultSet res = executeString("select * from external_hbase_mapped_table where rk > 20");
    +        assertResultSet(res);
    +        res.close();
     
    -      String expected = "col3,col2,rk\n" +
    -          "-------------------------------\n" +
    -          "96,{\"k1\":\"k1-96\", \"k2\":\"k2-96\"},96\n" +
    -          "97,{\"k1\":\"k1-97\", \"k2\":\"k2-97\"},97\n" +
    -          "98,{\"k1\":\"k1-98\", \"k2\":\"k2-98\"},98\n" +
    -          "99,{\"k1\":\"k1-99\", \"k2\":\"k2-99\"},99\n";
    +        //Projection
    +        res = executeString("select col3, col2, rk from external_hbase_mapped_table where rk > 95");
     
    -      assertEquals(expected, resultSetToString(res));
    -      res.close();
    +        String expected = "col3,col2,rk\n" +
    +            "-------------------------------\n" +
    +            "96,{\"k1\":\"k1-96\", \"k2\":\"k2-96\"},96\n" +
    +            "97,{\"k1\":\"k1-97\", \"k2\":\"k2-97\"},97\n" +
    +            "98,{\"k1\":\"k1-98\", \"k2\":\"k2-98\"},98\n" +
    +            "99,{\"k1\":\"k1-99\", \"k2\":\"k2-99\"},99\n";
     
    +        assertEquals(expected, resultSetToString(res));
    +        res.close();
    +
    +      } finally {
    +        executeString("DROP TABLE external_hbase_mapped_table PURGE").close();
    +        htable.close();
    +      }
         } finally {
    -      executeString("DROP TABLE external_hbase_mapped_table PURGE").close();
    -      htable.close();
    +      //TablespaceManager.addTableSpaceForTest(existing.get());
    --- End diff --
    
    Does this line need to be restored?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---