Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 4: (19 comments) http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java File fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java: PS4, Line 97: org.apache should this have changed? http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: PS4, Line 89: com.cloudera can you add a ref to IMPALA-4271 ? PS4, Line 160: know known PS4, Line 159: /** : * The number of nodes is not know ahead of time and will be updated during computeStats : * in the scan node. : */ : public int getNumNodes() { return -1; } I don't see this used PS4, Line 175: numClusteringCols_ = 0; not really related to this change, but it's kind of confusing to have numClusteringCols_ PS4, Line 226: List<FieldSchema> cols = msTable_.getSd().getCols(); : cols.clear(); why do we get cols from getCols() and then clear() it? PS4, Line 232: cols.add(new FieldSchema(colName, type.toSql().toLowerCase(), null)); why do we do this? cols isn't used later http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: PS4, Line 460: msTbl.getTableType().equals(TableType.EXTERNAL_TABLE.toString()); we shuold probably compare case insensitive to be safe http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: PS4, Line 1147: occurrs occurs PS4, Line 1482: } catch (Exception e) { : try { : // Error creating the table in HMS, drop the managed table from Kudu. : if (!Table.isExternalTable(newTable)) { : KuduCatalogOpExecutor.dropTable(newTable, false); : } : } catch (Exception logged) { : String kuduTableName = newTable.getParameters().get(KuduTable.KEY_TABLE_NAME); : LOG.error(String.format("Failed to drop Kudu table '%s'", kuduTableName), : logged); : throw new RuntimeException(String.format("Failed to create the table '%s' in " + : " the Metastore and the newly created Kudu table '%s' could not be " + : " dropped. The log contains more information.", newTable.getTableName(), : kuduTableName), e); : } : if (e instanceof AlreadyExistsException && params.if_not_exists) return false; : throw new ImpalaRuntimeException( : String.format(HMS_RPC_ERROR_FORMAT_STR, "createTable"), e); it looks like none of this really needs to be inside the synchronized block. Most of it doesn't matter but dropTable creates a KuduClient and makes an RPC. I could imagine this being a problem if Kudu is overwhelmed. You could catch the exception outside of the synchronized block, maybe some extra handling to know if it was created in HMS or not. Though from the looks of this we assume no exceptions from l1502 and 1503. http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: PS4, Line 232: if (!req.is_delta) { : catalog = new ImpaladCatalog(defaultKuduMasterAddrs_); : } 1line http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: PS4, Line 135: if (!hasRangePartitioning) { : tableOpts.setRangePartitionColumns(Collections.<String>emptyList()); : } I don't think this is necessary PS4, Line 175: erros errors PS4, Line 192: cols.clear(); can you indicate in the comment that this doesn't just populate msTbl's cols, it replaces whatever was there. PS4, Line 206: new KuduClient I'm not crazy about this wrapper class thing. It's only used in this file. PS4, Line 212: is accessible exists PS4, Line 215: validateTblProperties how about validateKuduTblExists ? PS4, Line 224: Error accessing table in Kudu " + : "master '%s' This could also print the name. Also to avoid confusing with potential future auth rules, can this say something like: "Kudu table TABLENAME on master MASTERADDRS does not exist." http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/util/KuduClient.java File fe/src/main/java/org/apache/impala/util/KuduClient.java: as I've said I'd vote to remove this, it's only used by 1 class and adds extra maintenance when we use new API methods or they change any. -- 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: 4 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
