You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2025/07/02 12:51:39 UTC
[Impala-ASF-CR] IMPALA-13850 (part 4): Implement in-place reset for CatalogD
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22640 )
Change subject: IMPALA-13850 (part 4): Implement in-place reset for CatalogD
......................................................................
Patch Set 41:
(7 comments)
The patch looks pretty good! I just have some minor comments.
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogResetManager.java
File fe/src/main/java/org/apache/impala/catalog/CatalogResetManager.java:
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogResetManager.java@63
PS41, Line 63: MAX_NUM_THREADS
Can we make this configurable? We might want to tune it in practise.
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogResetManager.java@266
PS41, Line 266: is currently is still in fetch queue or pending
: * queue
nit: remove "is still" ?
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogResetManager.java@283
PS41, Line 283: try (MetaStoreClient msClient = catalog_.getMetaStoreClient()) {
nit: Let's add a thread annotaion for this as well to show the db name.
http://gerrit.cloudera.org:8080/#/c/22640/41/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/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@477
PS41, Line 477: && getCatalogVersion() >= Catalog.CATALOG_VERSION_AFTER_FIRST_RESET
Why do we need this condition? At the begining catalog version is smaller than CATALOG_VERSION_AFTER_FIRST_RESET and I think we should wait here.
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2446
PS41, Line 2446: succes
nit: typo
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2470
PS41, Line 2470: throws MetaException, TException, InterruptedException, ExecutionException {
It seems no exceptions will be thrown in this method. All are caught on L2516.
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java
File fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java:
http://gerrit.cloudera.org:8080/#/c/22640/41/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@52
PS41, Line 52: protected static final int MAX_HMS_CONNECTION_POOL_SIZE = 32;
nit: not used in this patch?
--
To view, visit http://gerrit.cloudera.org:8080/22640
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4ae2154612746b34484391c5950e74b61f85c9d
Gerrit-Change-Number: 22640
Gerrit-PatchSet: 41
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <k....@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Jul 2025 12:51:39 +0000
Gerrit-HasComments: Yes