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

[hyunsik]

…pace.

[jihoonson]

Unused imports.

[jihoonson]

@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

[hyunsik]

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.

[jinossy]

@hyunsik
I recommend default handler to FileTablespace, if a handler not found. What do you think?

[hyunsik]

@jinossy In my opinion, the idea 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.

[jihoonson]

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?

[hyunsik]

@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.

[jinossy]

@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

[hyunsik]

The case where someone develops custom file system rarely occurs in real field. That's too much.

[jihoonson]

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.

[hyunsik]

Thank you for your comments. I've reflected your latest comment.

[jihoonson]

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.

[hyunsik]

Fixed.

[jihoonson]

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?

[hyunsik]

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.

[jihoonson]

+1 the latest patch looks good to me. I left a trivial comment. If you agree, please reflect before commit.