impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Date Tue, 11 Oct 2016 00:02:23 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message