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 2024/08/22 22:07:13 UTC
[Impala-ASF-CR] IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21714
Change subject: IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
......................................................................
IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
There are two HDFS APIs to list file metadata of a folder - listFiles
and listStatus. The first one returns a list of LocatedFileStatus which
has the info of block locations. The second one returns a list of
FileStatus without block locations. listStatus is usually used on
storages that don't have blocks, e.g. S3, or used in scenarios that we
want to list basic info (name, length, last modified time) reuse
existing file descriptors.
This patch simplifies the check on whether to list files with block
locations. When the table/partition is marked as HDFS caching enabled,
or if there are no existing file descriptors to reuse, we invoke the
listFiles API.
Also tracks whether there are file changes in FileMetadataLoader to
speed up the normal case in hasFilesChangedCompareTo().
Tests
- Ran CORE tests
Change-Id: Ib690930b57a161c98f07ccc2304860b788e9cd9f
---
M fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
5 files changed, 54 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/21714/1
--
To view, visit http://gerrit.cloudera.org:8080/21714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib690930b57a161c98f07ccc2304860b788e9cd9f
Gerrit-Change-Number: 21714
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21714 )
Change subject: IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16773/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib690930b57a161c98f07ccc2304860b788e9cd9f
Gerrit-Change-Number: 21714
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Aug 2024 22:37:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/21714 )
Change subject: IMPALA-13117: (part-1) simplify listWithLocations in FileMetadataLoader
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/21714/1/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java
File fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java:
http://gerrit.cloudera.org:8080/#/c/21714/1/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@321
PS1, Line 321: if (oldFds == oldFdsByPath_) return hasFilesChanged_;
We are comparing a list against a map. should this be
`oldFs == new ArrayList<>(oldFdsByPath_.values())`?
http://gerrit.cloudera.org:8080/#/c/21714/1/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java
File fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java:
http://gerrit.cloudera.org:8080/#/c/21714/1/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@285
PS1, Line 285: if (forceRefreshLocations_) return false;
From L#186, can there be a condition where forceRefreshLocations_ is false and (fd == null || fs.ischanged())?
If such a condition exists, then we should also add 'hasFilesChanged_' in this if condition.
--
To view, visit http://gerrit.cloudera.org:8080/21714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib690930b57a161c98f07ccc2304860b788e9cd9f
Gerrit-Change-Number: 21714
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Sep 2024 18:36:24 +0000
Gerrit-HasComments: Yes