Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20574 )
Change subject: IMPALA-7131: Support external data sources in LocalCatalog mode ...................................................................... Patch Set 4: (13 comments) http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3664 PS3, Line 3664: resp.setData_srcs(Lists.newArrayList()); : } else { > Why we can't return an empty list for this case? Change code to return empty list http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3675 PS3, Line 3675: resp.setData_srcs(Lists.newArrayList()); : } else { return empty list in this case. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3794 PS3, Line 3794: databa > nit: this should be "dbs". We don't send table names here. Fixed the comments http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3809 PS3, Line 3809: return resp; > Just curious, any other global info we are missing? It'd be nice to file sp Don't aware other types of global info. Removed the comments. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@671 PS3, Line 671: if (dsName == null) dsName = ""; : req.object_desc.setData_source(new TDataSource(dsName, "", "", "")); : return req; : } : : @Override : public Database loadDb(final String dbName) throws TException { : return loadWithCaching("database metadata for " + dbName, : DB_METADATA_STATS_CATEGORY, > nit: these can be simplified as Changed code as suggested. There is error if we don't set the fields of 'hdfs_location', 'class_name' and 'api_version' in the request. All fields of TDataSource are defined as required fields. If don't set other three fields as empty string, the object is not correctly transferred to server side. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1233 PS3, Line 1233: TGetPartialCatalogObjectResponse resp = sendRequest(req); > This seems unused. It's unused now. Plan to add more fields in TDataSource object after making DataSource object persistent, and change "show data sources" command to show only the names of DataSource objects. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java: http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@207 PS3, Line 207: > Can we ignore all the exceptions? I'm not sure what exceptions will be thro This function is defined not throw exception in FeCatalog so catch the exception here. It call into CatalogdMetaProvider.sendRequest(). sendRequest() could throw exception if there is RPC failure. Added log message. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@218 PS3, Line 218: } > Need logs for the exception here too Added log. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java: http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@20 PS3, Line 20: import java.util.Set; > nit: unused import removed http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@23 PS3, Line 23: import org.apache.impala.catalog.DataSourceTable; > nit: unused import removed http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@31 PS3, Line 31: import org.apache.impala.thrift.TDataSourceTable; > nit: unused import removed http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@37 PS3, Line 37: import org.slf4j.Logger; > nit: unused import removed http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@103 PS3, Line 103: > nit: duplicated "the" removed -- To view, visit http://gerrit.cloudera.org:8080/20574 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40841c9be9064ac67771c4d3f5acbb3b552a2e55 Gerrit-Change-Number: 20574 Gerrit-PatchSet: 4 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 19 Oct 2023 04:33:37 +0000 Gerrit-HasComments: Yes
