Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17284 )
Change subject: IMPALA-10645: Log catalogd HMS API metrics ...................................................................... Patch Set 2: (26 comments) Sorry for the formatting errors. Hopefully they are all gone now. http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@142 PS2, Line 142: catalog, reqBuilder.build(), dbName, tblName, HmsApiNameEnum.GET_TABLE_REQ.apiName()); > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@245 PS2, Line 245: catalogReq.build(), dbName, tblName, HmsApiNameEnum.GET_PARTITION_BY_EXPR.apiName()); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@367 PS2, Line 367: requestBuilder.build(), dbName, tblName, HmsApiNameEnum.GET_PARTITION_BY_NAMES.apiName()); > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/17284/2/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/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2221 PS2, Line 2221: > nit: I think we use 4 spaces idention here Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2227 PS2, Line 2227: > nit: 4 spaces idention Yeah, looks like the original patch from Kishen used wrong code-style template. Done. http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2230 PS2, Line 2230: if > nit: need one space after "if" Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2231 PS2, Line 2231: // Update the cache miss metric, as the valid write id list did not match and we have to reload the table. > line too long (114 > 90) Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2233 PS2, Line 2233: > nit: 4 spaces idention Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2067 PS2, Line 2067: Counter misses = CatalogMonitor.INSTANCE.getCatalogdHmsCacheMetrics().getCounter(FILEMETADATA_CACHE_MISS_METRIC); > Previously we count the metrics per table, this changes it to use a global Yes, I think changing this from table level to global level doesn't make a lot of sense. The metric value is not very useful if it is at global level. Let me see how can I change it to table level back. http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2068 PS2, Line 2068: Counter hits = CatalogMonitor.INSTANCE.getCatalogdHmsCacheMetrics().getCounter(FILEMETADATA_CACHE_HIT_METRIC); > line too long (114 > 90) Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2131 PS2, Line 2131: getFileMetadataCacheHitRate > After this patch, the hit rate will be a global hit rate counted on all tab I think we should keep it at table level. http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@77 PS2, Line 77: private static final Logger LOG = : LoggerFactory.getLogger(CatalogMetastoreServer.class); > nit: don't need changes since the original line fits in 90 chars. Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@94 PS2, Line 94: private static final String ACTIVE_CONNECTIONS_METRIC : = "metastore.active.connections"; > nit: don't need changes as well. Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@99 PS2, Line 99: = > nit: I think our code style tend to put this in the above line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@143 PS2, Line 143: > nit: redundant blank line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@200 PS2, Line 200: throws Throwable { > nit: move to the above line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@201 PS2, Line 201: > nit: redundant blank line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@202 PS2, Line 202: contains > nit: Unnecessary 'contains' check. We can always call add() directly. Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@256 PS2, Line 256: /** : * : */ > Do you plan to add something here? Yeah. Looks like the original patch from Kishen didn't have it. I have added it now. http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@486 PS2, Line 486: > nit: redundant blank line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@488 PS2, Line 488: > nit: redundant blank line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@122 PS2, Line 122: LOG.info(String.format(HMS_FALLBACK_MSG_FORMAT, HmsApiNameEnum.GET_PARTITION_BY_EXPR.apiName(), tblName)); > line too long (110 > 90) Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java File fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java@27 PS2, Line 27: > nit: this file uses 4 spaces indention. Could you adjust them to 2? Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java@43 PS2, Line 43: if > nit: need one space after "if" Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/ICatalogMetastoreServer.java File fe/src/main/java/org/apache/impala/catalog/metastore/ICatalogMetastoreServer.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/ICatalogMetastoreServer.java@47 PS2, Line 47: > nit: redundant blank line Done http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17284/2/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1304 PS2, Line 1304: LOG.info(String.format(HMS_FALLBACK_MSG_FORMAT, HmsApiNameEnum.GET_PARTITION_BY_NAMES.apiName(), tblName)); > line too long (111 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/17284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id41afe89bbe3395c158919bddd09f302c6752287 Gerrit-Change-Number: 17284 Gerrit-PatchSet: 2 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 14 Apr 2021 21:57:39 +0000 Gerrit-HasComments: Yes
