Casey Ching has posted comments on this change. Change subject: Simplify creating external Kudu tables and add DROP DATABASE CASCADE ......................................................................
Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/2617/10/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1113: // The use of DdlDelegates combined with the fact that the catalog is disconnected : // from the source of truth, the Hive Metastore, leads to some complications here. : // If 'db' is null, that should mean either the database doesn't really exist or the : // database was created outside of Impala and the user didn't run "invalidate : // metadata". If 'db' is not null, there still may be tables in the Metastore that : // would require a DdlDelegate. Another scenario is when the user creates tables : // that require a DdlDelegate in Impala, then drops the database in Hive. Ideally the : // user visible result of DROP DATABASE CASCADE would be the same in any case. The : // best solution is probably to move the DdlDelegate functionality into Hive. For : // now Impala will assume that any table not in its cache also doesn't require the : // use of a DdlDelegate (ignoring the no-op UnsupportedDDLDelegate class). If that : // assumption isn't correct, the database will still be dropped in the Metastore : // but the underlying data would remain. Users can issue a REFRESH command to load : // the database metadata before dropping to ensure delegates will be used when needed. > I feel this comment is still a bit confusing. First of all it lists 3 cases Matt was saying something similar. I'm just going to remove it then since it doesn't seem to be helpful. I dont think the code below needs a comment. The interesting part is describing the reasoning behind the "db == null" case and why there is no code here for that. That's what this comment is all about. http://gerrit.cloudera.org:8080/#/c/2617/10/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 1580: } > If you don't have that already can you add the following test cases: Those should be in the py tests. It's not really interesting here because only analysis happens and that doesn't actually contact Kudu at all. If things were broken in the cases above, that would be seen during table creation. http://gerrit.cloudera.org:8080/#/c/2617/10/tests/common/kudu_test_suite.py File tests/common/kudu_test_suite.py: Line 52: def get_db_name(cls): : # When py.test runs with the xdist plugin, several processes are started and each : # process runs some partition of the tests. It's possible that multiple processes : # will call this method. A random value is generated so the processes won't try : # to use the same database at the same time. The value is cached so within a single : # process the same database name is always used for the class. This doesn't need to : # be thread-safe since multi-threading is never used. : if not cls.__DB_NAME: : cls.__DB_NAME = \ : choice(ascii_lowercase) + "".join(sample(ascii_lowercase + digits, 5)) : return cls.__DB_NAME > Can't you use the db fixture that Michael created instead? That version is at the function level so its unnecessarily slow. The code is so small it seems fine to duplicate. Plus the features are different. Eventually it should all be consolidated though. http://gerrit.cloudera.org:8080/#/c/2617/10/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 230: > Can you add a test (if not exists) where a database that has kudu tables is This is what that big comment was about. Basically things are broken in those scenarios until Hive supports dropping Kudu tables. Adding tests to confirm that things are broken seems strange. -- To view, visit http://gerrit.cloudera.org:8080/2617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic141102818b6dad3016181b179a14024d0ff709d Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
