Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 1: (31 comments) Ok, part 3. I have one more batch left... http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java File fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java: PS1, Line 222: : : : : : : was this moved somewhere else? Maybe I'll find it as I keep reading... PS1, Line 238: // Update the HMS : if (reuseMetadata) { : try { : client.alter_table(msTable_.getDbName(), msTable_.getTableName(), msTable_); : } catch (TException e) { : throw new TableLoadingException(e.getMessage()); : } : } Same deal, I'm not sure why reuseMetadata means update the HMS. http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: PS1, Line 220: : private final static KuduCatalogOpExecutor kuduExecutor_ : = new KuduCatalogOpExecutor(); all static methods, why instantiate it? PS1, Line 1122: // The db == null case isn't handled. The only tables this should matter for are : // Kudu tables. The expectation is Impala will always know about any Kudu tables : // because Hive doesn't support Kudu yet. I've always had a problem with this comment :/ even after trimming it down I don't think it makes complete sense. This should mention briefly what this code _is handling_ before it talks about what it's not handling. Can you add 1 sentence at the beginning? PS1, Line 1124: When Hive supports Kudu the DDL delegates : // can be removed. https://issues.cloudera.org/browse/IMPALA-3424 tracks the removal. Can you remove these sentences and just leave IMPALA-3424 as a reference? We may not remove our kudu code even if/when Hive supports Kudu. PS1, Line 1127: , nit: extra comma PS1, Line 1150: // The operation will be aborted if the Kudu table cannot be dropped. If for : // some reason Kudu is permanently stuck in a non-functional state, the user is : // expected to ALTER TABLE to either set the table to UNMANAGED or set the format : // to something else. JIRA? Even if we don't fix it we should have something public to point to regarding this behavior. Also to make sure the docs team knows to document it. PS1, Line 1205: kuduExecutor_ static ref PS1, Line 1406: if (KuduTable.isKuduTable(tbl)) { : return createKuduTable(tbl, params.if_not_exists, params.getDistribute_by(), : response); : } : return createTable(tbl, params.if_not_exists, params.getCache_op(), response); : } it's too bad we have to just branch like this and handle everything differently, but I can't think of anything better. PS1, Line 1420: createMetaStoreTable I'd prefer to have this on the prev line and the parameter on the new line Line 1475: * creation of a managed Kudu table. comment on response param PS1, Line 1480: TDdlExecResponse response) I think this just fits on the prev line PS1, Line 1483: kuduExecutor_ static reference KuduCatalogOpExecutor, here elsewhere in this fn PS1, Line 1509: // Add the table to the catalog cache : Table newTbl = catalog_.addTable(newTable.getDbName(), newTable.getTableName()); : addTableToCatalogUpdate(newTbl, response.result); While I don't think these will throw, it might be worth wrapping all the logic after the Kudu create table in a try { } catch block that drops the kudu table. That'll future proof this a bit. Feel free to ignore though. http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: PS1, Line 230: impaladCatalog_.getDefaultKuduMasterAddrs() this is fine but a little weird, normally I'm all about removing extra state but in this case it might be better to keep this as a parameter on frontend and pass it in. Unless there's somewhere else besides the old impaladCatalog we can get it? http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/service/JniCatalog.java File fe/src/main/java/com/cloudera/impala/service/JniCatalog.java: PS1, Line 83: : int otherLogLevel, boolean allowAuthToLocal, String kerberosPrincipal) wrapping http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/KuduCatalogOpExecutor.java: PS1, Line 29: import org.apache.hadoop.hive.metastore.api.T since this file references our Table class we should import ours and reference this one w/ namespace. that's just our convention. PS1, Line 45: yet remove PS1, Line 45: When Hive : * functionality is available, that should be preferred to the functionality here. : * https://issues.cloudera.org/browse/IMPALA-3424 tracks this. remove text since we might not. JIRA can stay but is actually a different issue IMO. PS1, Line 65: com.cloudera.impala.catalog.Table. shouldn't need to ref the full namespace Line 149: * Reads the schema from a Kudu table and populates 'msTbl' with an equivalent schema. If an error occurs we may have partially modified the table and leave it in an inconsistent state. PS1, Line 150: if unable to do so. if any errors are encountered. PS1, Line 155: // Schemas for external tables are not specified by the user in DDL. Instead the : // user provides a Kudu table name (or uses implicit Hive Metastore to Kudu mapping : // rules), then the schema is imported from Kudu. I'm not sure if this is adding much and I think it's a bit confusing/out of place. PS1, Line 162: // Start searching.... remove PS1, Line 166: if (!Strings.isNullOrEmpty(dbName) && !dbName.equalsIgnoreCase("default")) { : // If it exists, the fully qualified name is preferred. : candidateTableNames.add(dbName + "." + tableName); : } i don't think we should bother handling the default case specially. Can we just always handle it as dbname.tableName? ...and drop the 'candidate' below? http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/util/KuduClient.java File fe/src/main/java/com/cloudera/impala/util/KuduClient.java: PS1, Line 30: * This class wraps an org.apache.kudu.client.KuduClient to transform exceptions into : * ImpalaRuntimeExceptions. No additional functionality is provided. See the Kudu : * documentation for information about the methods. Let's think about whether or not we can avoid this. Obviously this will have to be updated when kudu client changes... blegh. http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/util/KuduUtil.java File fe/src/main/java/com/cloudera/impala/util/KuduUtil.java: PS1, Line 41: org.apache.kudu. reference the ext Type and import ours PS1, Line 191: com.cloudera.impala.catalog. import? PS1, Line 191: isSupportedKeyType is this their restriction? PS1, Line 196: and the user did : * not provide an explicit Kudu table name ... a table, assuming a custom name was not provided. PS1, Line 201: Catalog.isDefaultDb(metastoreDbName) ? you know i'm voting for not special casing this... -- To view, visit http://gerrit.cloudera.org:8080/4414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
