You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org> on 2016/11/19 02:00:08 UTC
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Bharath Vissapragada has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5148
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use to improvement to reduce an addition
RPC to NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
2 files changed, 255 insertions(+), 173 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/1
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 15: Code-Review+2
(2 comments)
Nice!
http://gerrit.cloudera.org:8080/#/c/5148/15/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
PS15, Line 794: for (Path location: locations) {
: loadBlockMetadata(fs, location, partsByPath);
: }
nit: single line?
PS15, Line 1034: while reusing existing file descriptors to avoid loading metadata for files
: * that haven't changed.
This is no longer valid. Remove?
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#3).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use to improvement to reduce an addition
RPC to NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
3 files changed, 385 insertions(+), 250 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/3
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#11).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
4 files changed, 358 insertions(+), 341 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/11
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#5).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
3 files changed, 387 insertions(+), 250 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/5
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Re: [Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block
metadata loading
Posted by Dimitris Tsirogiannis <dt...@cloudera.com>.
Looking into it now.
Dimitris
On Tue, Nov 29, 2016 at 10:12 PM, Bharath Vissapragada (Code Review)
<ge...@cloudera.org> wrote:
> Bharath Vissapragada has posted comments on this change.
>
> Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
> ......................................................................
>
>
> Patch Set 13: Code-Review+1
>
> Thanks for the review Alex. Carrying +1.
>
> --
> To view, visit http://gerrit.cloudera.org:8080/5148
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: comment
> Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
> Gerrit-PatchSet: 13
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
> Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
> Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
> Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
> Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
> Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 13: Code-Review+1
Thanks for the review Alex. Carrying +1.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#18).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
6 files changed, 367 insertions(+), 341 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/18
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 13:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
PS13, Line 41: private DiskIdMapper() {
: }
nit: single line
PS13, Line 44: "storage ID UUID string"
nit: why quoted?
Line 55: * Returns a disk id (0-based) index for storageId on host 'host'.
This function doesn't only return a disk id (implying this is a getter), it also generates and adds a disk id for a non-existent <host, storageUuid> pair. That needs to be documented.
PS13, Line 68: .intValue()
why do you need this? Won't this be unboxed automatically?
PS13, Line 69: synchronized (storageIdGenerator) {
The common path now includes two calls to storageUuidToDiskId.get(), one synchronized block. Won't it be better if you make this function synchronized and remove the test and test and set logic? In this case you only need one call to storageUuidToDiskId.get(), this doesn't need to be a concurrent hash map and you still have 1 synchronized block with the same number of operations in it.
PS13, Line 74: stoprageUuid
typo: storageUuid
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
PS13, Line 315: if (!FileSystemUtil.isChildPath(partDir, dirPath)) continue;
Is this check really needed? Haven't you done this check while populating partsByPath?
PS13, Line 316: perPartitionFileDescMap_
If I recall correctly, I added this to speedup incremental loading of file descriptors for files that already existed in the metadata and for which their blocks hadn't changed. My understanding is that this code is no longer here (i.e. we already reload the file/block metadata from scratch). So, is this map still needed?
PS13, Line 431:
nit: extra space
PS13, Line 441: // Synthesize the block metadata for the file descriptor.
: long start = 0;
: long remaining = fd.getFileLength();
: // Workaround HADOOP-11584 by using the filesystem default block size rather than
: // the block size from the FileStatus.
: // TODO: after HADOOP-11584 is resolved, get the block size from the FileStatus.
: long blockSize = fs.getDefaultBlockSize();
: if (blockSize < MIN_SYNTHETIC_BLOCK_SIZE) blockSize = MIN_SYNTHETIC_BLOCK_SIZE;
: Preconditions.checkState(partitions.size() > 0);
: // For the purpose of synthesizing block metadata, we assume that all partitions
: // with the same location have the same file format.
: HdfsFileFormat fileFormat = partitions.get(0).getFileFormat();
: if (!fileFormat.isSplittable(HdfsCompression.fromFileName(fd.getFileName()))) {
: blockSize = remaining;
: }
: while (remaining > 0) {
: long len = Math.min(remaining, blockSize);
: List<BlockReplica> replicas = Lists.newArrayList(
: new BlockReplica(hostIndex_.getIndex(REMOTE_NETWORK_ADDRESS), false));
: fd.addFileBlock(new FileBlock(start, len, replicas));
: remaining -= len;
: start += len;
: }
I would put this in a separate function.
PS13, Line 474: numHdfsFiles_++;
: totalHdfsBytes_ += partition.getSize();
Is this counting correct here?
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
PS13, Line 289: BlockLocation
Is this the name of the API call? If not, can you use the exact function name?
PS13, Line 418: child
child or descendant?
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#19).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
6 files changed, 367 insertions(+), 341 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/19
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 17: Code-Review+2
- Hdfs core exhaustive failed on a single test "test_cache_reload_validation". PS17 fixes that. The bug there was that we don't call HdfsCachingUtil.validateCacheParams() on msPart objects and hence it contains stale caching metadata, even if the cache directives were deleted from hdfs (outside of Impala).
- Couldn't get S3 tests working. Looking into it.
Trivial change, carrying +2.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5148/15/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
PS15, Line 794: for (Path location: locations) {
: loadBlockMetadata(fs, location, partsByPath);
: }
> nit: single line?
Done
PS15, Line 1034: while reusing existing file descriptors to avoid loading metadata for files
: * that haven't changed.
> This is no longer valid. Remove?
Done
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
PS13, Line 69: synchronized (storageIdGenerator) {
> The common path now includes two calls to storageUuidToDiskId.get(), one sy
The common case is that at some point this mapping will be complete, and most requests will be served by doing a get() on the concurrent hashmap. The CC hashmap does not lock on get(), i.e., in the common case we can get away with any locking.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#13).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 379 insertions(+), 345 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/13
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#14).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 359 insertions(+), 335 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/14
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Mostafa Mokhtar (Code Review)" <ge...@cloudera.org>.
Mostafa Mokhtar has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 4:
Just tried out the latest patch and metadata loading is 5.4x faster.
With the patch metadata loading for 80 partitions with 250K files finished in 27 seconds compared to 146 seconds without.
Most of the CPU time is spent in the RemoteIterator, to further speedup metadata loading I recommend using a thread pool.
Stack Trace Sample Count Percentage(%)
org.apache.impala.catalog.HdfsTable.load(boolean, IMetaStoreClient, Table) 509 74.307
org.apache.impala.catalog.HdfsTable.load(boolean, IMetaStoreClient, Table, boolean, boolean, Set) 509 74.307
org.apache.impala.catalog.HdfsTable.loadAllPartitions(List, Table) 507 74.015
org.apache.impala.catalog.HdfsTable.loadMetadataAndDiskIds(FileSystem, List, HashMap) 497 72.555
org.apache.impala.catalog.HdfsTable.loadBlockMetadata(FileSystem, Path, HashMap, Map) 472 68.905
org.apache.hadoop.fs.FileSystem$5.hasNext() 365 53.285
org.apache.hadoop.hdfs.DistributedFileSystem$DirListingIterator.hasNext() 339 49.489
org.apache.hadoop.hdfs.DistributedFileSystem$DirListingIterator.hasNextNoFilter() 258 37.664
org.apache.hadoop.hdfs.DFSClient.listPaths(String, byte[], boolean) 258 37.664
com.sun.proxy.$Proxy21.getListing(String, byte[], boolean) 258 37.664
org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(Object, Method, Object[]) 258 37.664
org.apache.hadoop.io.retry.RetryInvocationHandler.invokeMethod(Method, Object[]) 258 37.664
java.lang.reflect.Method.invoke(Object, Object[]) 258 37.664
org.apache.hadoop.hdfs.protocol.HdfsLocatedFileStatus.makeQualifiedLocated(URI, Path) 81 11.825
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 17:
Was just going through this patch. I think it's necessary to run some tests on a table with partitions on multiple filesystems as well. Since our test infra doesn't allow for that currently, it has to be done manually.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#7).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
4 files changed, 393 insertions(+), 260 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/7
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 368: if (!pathMd.filteredParitionPaths.contains(fileStatus.getPath())) continue;
> Tried out the patch and the ArrayList is very inefficient and causing a hug
I've discussed this with Alex yesterday. I'm redoing the logic here to cut down another round of RPCs and also get rid of these ArrayLists. Thanks for doing a perf run.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 19: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 351: Map<String, Map<String, FileDescriptor>> newFileDescMap = Maps.newHashMap();
> Before, we used to want to reuse as much of the existing block metadata as
Done
Line 386: String[] blockHostPorts = loc.getNames();
> The code and comments around these is very sparse on content, so I think we
Did an initial round of cleanup. Removed all the Preconditions as they seem impossible. Also this code is on a hot path, so might help save some CPU.
Line 934: boolean isMarkedCached = isMarkedCached_;
> Looks like we used to force reloading the block locations of cached partiti
Done. It doesn't depend on isMarkCached anymore. This particular line is deadcode. We actually set partition caching properties in HdfsPartition c'tor by parsing msPartition object. This was required earlier because updatePartitionFds needed it.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 4:
(29 comments)
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 37: private static DiskIdMapper diskIdMapper = null;
why not:
public static DiskIdMapper INSTANCE = new DiskIdMapper()?
Line 50: private static HashMap<String, Integer> storageIdToInt = Maps.newHashMap();
make members and methods non-static since we are using a singleton
Line 63: public static int getDiskId(String host, String storageId) {
storageUuid
Line 72: if (storageIdToInt.containsKey(storageId)) {
I did some more reading and I'm afraid this "double checked locking" scheme does not work correctly in Java. The readers could return a ConcurrentModificationException or even corrupted data.
See for example:
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
I think we should make storageIdGenerator a ConcurrentHashMap to address the issue.
You can avoid doing two lookups here by doing get() and then checking for null.
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 120: import java.io.IOException;
I think these might need cleanup.
Line 351: Map<String, Map<String, FileDescriptor>> newFileDescMap = Maps.newHashMap();
What's the benefit of populating this temporary map and then merging it into the global one? Why does updating the global one directly not work?
Line 382: // Loop over all blocks in the file.
comment doesn't add anything, remove
Line 384: Preconditions.checkNotNull(loc);
Not your change, but this looks weird, why would it be null? Remove?
Line 386: String[] blockHostPorts = loc.getNames();
Let's just use loc.getNames() and loc.getHosts() inside the loop in L403. I don't see any benefit to having these as separate variables and their surrounding comments, do you?
Line 395: Sets.newHashSet(Arrays.asList(loc.getCachedHosts()));
I think we can get rid of the asList() here
Line 399: // to hostMap_/hostList_. The host ID (index in to the hostList_) for each
update comment, "hostMap_" doesn't seem to exist
Line 413: // Remember the THdfsFileBlocks and corresponding BlockLocations. Once all the
is this comment still accurate?
Line 423: for (List<HdfsPartition> partLists: filteredParts.values()) {
Seems cheaper to loop on newFileDescMap on look up filteredParts
Line 431: numHdfsFiles_ += fileDescs.size();
Why do the number of files and bytes always keep increasing?
Line 458: fileStatus.getModificationTime());
indent 4 (and same issue elsewhere)
Line 468: // All the partitions corresponding to the same partition path, have the same
I don't think this is necessarily true, but you can state that we assume it is so.
Line 483: for (HdfsPartition partition: partitions) {
This function looks much more expensive than before. We loop over all partition paths in L451 and here we loop over all partitions, so that's O(N^2) in the number of partitions.
Line 780: Map<FsKey, FileBlocksInfo> blocksToLoad = Maps.newHashMap();
unused?
Line 784: HashMap<Path, List<HdfsPartition>> filteredParts = Maps.newHashMap();
The var name filteredParts is a little confusing to me. How about partsByPath or something like that. The "filtered" part is a little unclear.
Line 830: if (filteredParts.containsKey(partDir)) filteredParts.get(partDir).add(partition);
use {}
use get() and check for null to avoid multiple lookups
Line 833: // Check if the partition directory's is a child dir of hdfsBaseDir_
typo: directory's -> directory
Line 835: !FileSystemUtil.isChildPath(partDir, getHdfsBaseDirPath())) {
use tblLocation, that's clearer (and in comment above)
Line 836: // Add the partition to the list of paths to load block MD.
Comment only describes the code below, maybe say something like:
// This partition has a custom filesystem location. Load its file/block metadata separately by adding it to the list of dirs to load.
Line 868: loadBlockMetadata(fs, location, filteredParts, fileBlocksToLoad);
Having separate loadBlockMetadata() and loadDiskIds() doesn't seem to make much sense. We're spending a lot of effort to populate the temporary fileBlocksToLoad map just so we can maintain the separation (which does not seem to make sense).
Is there a reason why we could not want to load the block md and disk ids at the same time (without this tmp map)?
Line 934: boolean isMarkedCached = isMarkedCached_;
looks unused, are you sure nothing got broken here?
Line 953: try {
not your change, but you can do this instead:
for (Expr v: keyValues) v.analyzeNoThrow(null);
Line 1583: boolean isMarkedCached = isMarkedCached_;
looks unused, sure nothing is broken here?
Line 1628: totalHdfsBytes_ += hdfsPart.getSize();
Why can't this stay inside addPartition()? In the loading case the size and number of file descriptors should be 0, so adding that should be harmless.
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 423: if (p.isRoot()) return false;
isn't the equals() below sufficient?
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Mostafa Mokhtar (Code Review)" <ge...@cloudera.org>.
Mostafa Mokhtar has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5148/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 347: LOG.debug("Loading block md for " + name_ + " directory " + dirPath.toString());
Please add If(LOG.isDebugEnabled()) to avoid calling the dirPath.toString() as this causes lots of unnecessary allocations which causes GC pauses .
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 2:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 27: * This class encapsulates the logic required for mapping HDFS
> Much fluff, simplify to: Maps HDFS storage-UUIDs to per-host 0-based, seque
Done
Line 29: * by the backend.
> required by Impala backends
Done
Line 31: public class DiskIdMapper {
> Make this a singleton to clarify the intended use.
Done
Line 36: // This is intentionally implemented as a static object to share the storageIDs across
> move this part to the class comment
Done
Line 38: // consistent across all the tables, without which, we can potentially confuse the
> also mention that the static solution requires the least amount of memory
Done
Line 48: private static ConcurrentHashMap<String, AtomicInteger>
> Don't think we need a ConcurrentHashMap or AtomicInteger here, because when
Used a stricter locking scheme, but as we discussed offline, this can still have contention in the initial metadata load. Added a TODO for that.
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 294: // Keeps track of all the parition paths that are filtered to add block metadata.
> this comment style for classes and functions
Oops, done.
Line 368: if (!pathMd.filteredParitionPaths.contains(fileStatus.getPath())) continue;
> Looks like this contains() and the indexOf() below can become very expensiv
The new PS design revamps the whole thing. We don't do indexOf() searches anymore.
Line 423: for (int i=0; i < fileMd.pathFds.size(); ++i) {
> i = 0 (add spaces)
Done
Line 458: // Only build the storage IDs fs instances that support the BlockLocation API.
> ... for fs instances ...
Done
Line 720: private void loadAllPartitions(
> After reading through this a few times I'm still confused about the high-le
The new PS implements a similar logic. We can sync-up on that if required.
Line 777: if (dirsToLoad.contains(partDir) ||
> positive is more readable to me
Done
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 275: * Returns true if the filesystem supports BlockLocation API.
> What API is it specifically? LocatedFileStatus.getBlockLocations()?
Yes, all methods returning 'BlockLocation's.
Line 277: public static boolean supportsBlockLocationAPI(FileSystem fs) {
> supportsBlockLocationApi
Done
Line 406: public static boolean isPathChild(Path p1, Path p2) {
> isChildPath(Path p, Path parent)
Done
Line 408: Path parentDir = p1.getParent();
> I think this can be improved by using the Path.getDepth(). You can do getPa
good idea. Done.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 12: Code-Review+1
(7 comments)
I'm pretty happy with the changes. Let's let Dimitris take a look.
http://gerrit.cloudera.org:8080/#/c/5148/12//COMMIT_MSG
Commit Message:
Line 15: We loop throuh each and every file in the table/partition directories
through
Line 17: This results in large no. of RPC calls to namenode, especially with
no. -> number
to the NameNode
especially for large tables
Line 35:
mention the behavior change of REFRESH
http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 31: * - To maintain consistent mapping across all the table instances so that the disk thread
... a consistent mapping ... so that the assignment of scan ranges to I/O threads is balanced and consistent for all scans on the same host.
http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 288: * Queries the filesystem to load the file block metadata (e.g. DFS blocks) for the
Suggest rephrasing:
Drops and re-loads the block metadata for all partitions in 'partsByPath' whose location is under the given 'dirPath'. It involves the following steps:
...
Line 296: * and enumerate all its blocks and their corresponding hosts and disk IDs.
remove part about disk ids, I think that's the next point
Line 778: LOG.debug("partsByPath size: " + partsByPath.size());
check log lvl
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#8).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
4 files changed, 371 insertions(+), 266 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/8
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5148/9/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 38: private static DiskIdMapper INSTANCE = new DiskIdMapper();
just make it public
http://gerrit.cloudera.org:8080/#/c/5148/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 403: for (List<HdfsPartition> partLists: partsByPath.values()) {
It seems simpler to move this logic into the while loop above and update the partition files and global stats incrementally. It will probably also be faster (look at partition.getSize()), but my main motivation is to make the code simpler to understand.
Since we now all the info we need in every LocatedFileStatus it seems we can map that file to its corresponding partition(s), and incrementally update all stats and auxiliary data structures.
Line 417: loadDiskIds(perFsFileBlocks);
I think we can get rid of this function and move the logic into the while loop above. We can get rid of the classes FsKey and FileBlocksInfo.
Same reason as above: Let's make this code simpler.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Mostafa Mokhtar (Code Review)" <ge...@cloudera.org>.
Mostafa Mokhtar has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 368: if (!pathMd.filteredParitionPaths.contains(fileStatus.getPath())) continue;
> Looks like this contains() and the indexOf() below can become very expensiv
Tried out the patch and the ArrayList is very inefficient and causing a huge regression.
Stack Trace Sample Count Percentage(%)
org.apache.impala.catalog.HdfsTable.loadBlockMetadata(FileSystem, Path, HdfsTable$FilteredPartitionPathsMd, Map) 19,323 99.974
java.util.ArrayList.contains(Object) 9,831 50.864
java.util.ArrayList.indexOf(Object) 9,458 48.934
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#10).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
4 files changed, 359 insertions(+), 341 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/10
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#9).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
4 files changed, 371 insertions(+), 266 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/9
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 13:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
PS13, Line 41: private DiskIdMapper() {
: }
> nit: single line
Done
PS13, Line 44: "storage ID UUID string"
> nit: why quoted?
Unquoted
Line 55: * Returns a disk id (0-based) index for storageId on host 'host'.
> This function doesn't only return a disk id (implying this is a getter), it
Made the comment little detailed.
PS13, Line 68: .intValue()
> why do you need this? Won't this be unboxed automatically?
Done
PS13, Line 69: synchronized (storageIdGenerator) {
> The common path now includes two calls to storageUuidToDiskId.get(), one sy
Discussed in person. It was put outside the synchronized block so that callers with already mapped storage ids needn't enter the synchronized block.
PS13, Line 74: stoprageUuid
> typo: storageUuid
Done
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
PS13, Line 315: if (!FileSystemUtil.isChildPath(partDir, dirPath)) continue;
> Is this check really needed? Haven't you done this check while populating p
Currently we don't do it at the callers. So I added the check here. partsByPath includes all the partitions that need to be updated. However this method does only for descendants of dirPath.
PS13, Line 316: perPartitionFileDescMap_
> If I recall correctly, I added this to speedup incremental loading of file
Looking closely, yes this map doesn't make sense anymore. We are unnecessarily doing puts and gets into it. Removed.Nice catch btw.
PS13, Line 431:
> nit: extra space
Done
PS13, Line 441: // Synthesize the block metadata for the file descriptor.
: long start = 0;
: long remaining = fd.getFileLength();
: // Workaround HADOOP-11584 by using the filesystem default block size rather than
: // the block size from the FileStatus.
: // TODO: after HADOOP-11584 is resolved, get the block size from the FileStatus.
: long blockSize = fs.getDefaultBlockSize();
: if (blockSize < MIN_SYNTHETIC_BLOCK_SIZE) blockSize = MIN_SYNTHETIC_BLOCK_SIZE;
: Preconditions.checkState(partitions.size() > 0);
: // For the purpose of synthesizing block metadata, we assume that all partitions
: // with the same location have the same file format.
: HdfsFileFormat fileFormat = partitions.get(0).getFileFormat();
: if (!fileFormat.isSplittable(HdfsCompression.fromFileName(fd.getFileName()))) {
: blockSize = remaining;
: }
: while (remaining > 0) {
: long len = Math.min(remaining, blockSize);
: List<BlockReplica> replicas = Lists.newArrayList(
: new BlockReplica(hostIndex_.getIndex(REMOTE_NETWORK_ADDRESS), false));
: fd.addFileBlock(new FileBlock(start, len, replicas));
: remaining -= len;
: start += len;
: }
> I would put this in a separate function.
Done
PS13, Line 474: numHdfsFiles_++;
: totalHdfsBytes_ += partition.getSize();
> Is this counting correct here?
We increment them for every fd (and its corresponding partition). We seem to be implementing reload() as drop() + add(). dropPartition() is subtracting them when required. Also loadPartitionFileMetadata() subtracts when we reload a subset of partitions. Do you think anything could be done better here? I agree its confusing since its done in multiple places.
http://gerrit.cloudera.org:8080/#/c/5148/13/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
PS13, Line 289: BlockLocation
> Is this the name of the API call? If not, can you use the exact function na
Rephrased it a little. Its no specific API call. Its just that only 'DistributedFileSystem's actually set storageIds in the BlockLocations and others don't.
PS13, Line 418: child
> child or descendant?
Descendant. Updated the method name and callers.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 16: Code-Review+2
Carrying +2. I'm running S3 core/hdfs core/hdfs exhaustive tests on this just to make sure we aren't breaking anything. I'll run the GVO once they pass. Thanks Dimitris and Alex.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#15).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
6 files changed, 366 insertions(+), 336 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/15
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#2).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use to improvement to reduce an addition
RPC to NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
3 files changed, 304 insertions(+), 181 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/2
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 11:
(40 comments)
http://gerrit.cloudera.org:8080/#/c/5148/11/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 29: * ids. This is internally implemented as a static object to share the storageIDs across
Several sentences Starting with "This", sometimes not clear what "This" references.
Would be nice to list the reasons in bullet point, something like this.
The storage UUID to disk id is global (shared across tables) for the following reasons:
1. Mention consistent scan range to disk thread assignment
2. Mention memory benefit
3. any more?
Line 44: // sequential 0-based integer disk id used by the hdfs scanners. This assumes that
used by the BE scanners.
Line 46: private ConcurrentHashMap<String, Integer> storageIdToInt =
storageUuidToDiskId
Line 50: // with the corresponding latest 0-based integer ID. Given a storage UUID, we first
with -> to
Line 51: // look in 'storageIdtoInt' map if it already exists, else we generate a new ID for
no need for this second sentence, that's what we do in getDiskId()
Line 64: if (Strings.isNullOrEmpty(storageUuid)) return diskId;
let's move this check to the caller and require the storageUuid to be non-null and non-empty in here
Line 68: // across the cluster, it is possible that we have a good hit rate.
it is possible -> we expect to have a good hit rate.
Line 72: // At this point, it could be possible due to race, that storageId might've been
// Mapping might have been added by another thread that entered the synchronized block first.
Line 76: // No mapping exists, create a new disk ID for 'storageId'
stoprageUuid
Line 80: // Host hasn't been populated yet. Start with a 0-based id.
// First disk id of this host.
http://gerrit.cloudera.org:8080/#/c/5148/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 289: * given directory 'dirPath'. Only blocks corresponding to paths from 'partsByPath'
Mention that the block metadata is effectively dropped and re-created.
Please provide a brief summary of the steps involved in this function. something like:
1. Clear block metadata of partitions
2. Call HDFS to list all files
3. etc.
Line 307: RemoteIterator<LocatedFileStatus> fileStatusIter = fs.listFiles(dirPath, true);
move right before the while loop
Line 308: // Reset the current state of partitions under dirPath from perPartitionFileDescMap_
Reset -> Clear
Line 310: for (Path partDir: partsByPath.keySet()) {
Iterate over the entrySet(), you are using an unnecessary get(partDir) below.
Line 321: String fileName = fileStatus.getPath().getName();
move closer to where it's used, i.e. right above L336
Line 322: if (!FileSystemUtil.isValidPartitionFile(fileStatus)) continue;
isValidDataFile()
Line 323: // At this point, file status points to a valid file under the directory. Now we
// Find the partition that this file belongs (if any).
Line 325: Path partPathDir = fileStatus.getPath().getParent();
move closer to where it's used, i.e. L357
Line 330: // Skip if this file doesn't correspond to any of the existing partitions.
Skip if this file does not belong to any known partition.
Line 332: LOG.debug("File " + fileStatus.getPath().toString() + " doesn't correspond " +
add if to check log level
Line 333: " to a valid partition. Skipping metadata load for this file.");
to a known partition
Line 361: Preconditions.checkState(perPartitionFileDescMap_.containsKey(partPathDirName));
avoid duplicate lookup with get() + Preconditions.checkNotNull()
Line 363: // Update the partitions metadata corresponding to this file descriptor
partitions' metadata that this file belongs to.
Line 371: LOG.warn("Unknown disk id count for filesystem " + fs + ":" + unknownDiskIdCount);
add if to check log level
Line 374: throw new RuntimeException("Error loading block md for directory "
block metadata
Line 380: * Loads the disk IDs for BlockLocation 'location' and its corresponding file block.
Comment should mention the storage UUID to disk id mapping.
Line 386: String[] hosts;
Do you know if these are the unique hosts? I'm wondering if storageIds and hosts are guaranteed to have the same length.
Line 394: for (int i =0; i < storageIds.length; ++i) {
int i = 0; (space after "=")
Line 410: for (Path partPath: partsByPath.keySet()) {
iterate over Map.Entry, you are doing a get() in the next line
Line 412: // For each valid file in the partPath, synthesize the block metadata.
For each data file in the partPath...
Line 427: // We assume all the partitions corresponding to the same partition path, have
For the purpose of synthesizing block metadata, we assume that all partitions with the same location have the same file format.
Line 447: perPartitionFileDescMap_.put(partition.getLocation(), fileDescMap);
move put() inside the if (fileDescMap == null) block
Line 673: * Create HdfsPartition objects corresponding to 'msPartitions' and add them to this
Briefly list the high-level steps, in particular that we first create the partition objects based on the HMS partitions, and then in a second step we populate the partitions' file metadata
Line 742: // Check if the partition directory is a child dir of tbl directory location.
remove, comment only states what the code does
Line 752: // Load the block metadata
remove comment (doesn't add anything)
Line 764: throw new CatalogException("Error loading block location md for partition " +
md -> metadata
Line 827: public HdfsPartition createPartition(StorageDescriptor storageDescriptor,
change this to createAndLoadPartition()? then we can get rid of the flag in createPartition() and this function becomes createPartition() + loadMetadataAndDiskIds()
http://gerrit.cloudera.org:8080/#/c/5148/11/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 276: * Returns true if the file corresponding to 'fileStatus' is a valid file as per
is a valid data file
Line 278: * directory or a hidden file starting with . or _ or if it is a LZO index file.
hidden file is enough, no need to mention specific prefixes
Line 291: public static boolean supportsBlockLocationApi(FileSystem fs) {
supportsBlockLocations()?
The API part is confusing because we are not really referencing a specific API. If you prefer to reference the API, then let's reference a specific method.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 6:
(29 comments)
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 37:
> why not:
Done
Line 50: // the storage ID of a particular disk is unique across all the nodes in the cluster.
> make members and methods non-static since we are using a singleton
Done
Line 63: * the initial metadata load. Figure out ways to fix it using finer locking scheme.
> storageUuid
Done
Line 72: // amount of data loading is done and storageIdtoInt is populated with storage IDs
> I did some more reading and I'm afraid this "double checked locking" scheme
Done
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 120: import java.io.IOException;
> I think these might need cleanup.
My editor is messing up these imports. Let me try fixing them by hand.
Line 351: Map<String, Map<String, FileDescriptor>> newFileDescMap = Maps.newHashMap();
> What's the benefit of populating this temporary map and then merging it int
I did that initially but the problem happens when one of the files is deleted. For example, consider the following sequence of actions.
- create partition p1 with file f1
- perPartitionFileDescMap_ contains f1
- insert overwrite p1 select * from random_table, say this overwrites f1 and creates f2.
- In such a case, we loop p1, and add f2 to perPartitionFileDescMap_ but we still have f1 at the end of it.
- So we need to scan all entries in perPartitionFileDescMap_ and figure out the difference with listStatus() and remove them. This is a costly operation.
- Easier way is to build a newFdMap with the current snapshot of listStatus() and replace it in perPartitionFileDescMap_
Also this way I felt the code is more readable. This is arguable, but I felt so.
Line 382: // Loop over all blocks in the file.
> comment doesn't add anything, remove
Done
Line 384: Preconditions.checkNotNull(loc);
> Not your change, but this looks weird, why would it be null? Remove?
Done
Line 386: String[] blockHostPorts = loc.getNames();
> Let's just use loc.getNames() and loc.getHosts() inside the loop in L403. I
We are using them in preconditions check in L396 and loop condition in L403 and L405,L408 for index based access. Let me know if you still think we should remove it.
Line 395: Sets.newHashSet(Arrays.asList(loc.getCachedHosts()));
> I think we can get rid of the asList() here
nice catch. done
Line 399: // to hostMap_/hostList_. The host ID (index in to the hostList_) for each
> update comment, "hostMap_" doesn't seem to exist
Remnants of old clean-up. This logic is now merged into hostIndex_. Updated comment accordingly.
Line 413: // Remember the THdfsFileBlocks and corresponding BlockLocations. Once all the
> is this comment still accurate?
Yes, we basically add all these blocks to perFsFileBlocks and run getDiskId() on it in bulk.
Line 431: numHdfsFiles_ += fileDescs.size();
> Why do the number of files and bytes always keep increasing?
I think its due to the current design. The reload is implemented as dropPartition() + creatPartition(). Also we resetPartitions() at few places to make it 0. Basically we never delete entries from it, we just overwrite them with new values.
Line 458: fileStatus.getModificationTime());
> indent 4 (and same issue elsewhere)
Fixed my editor settings now. Done.
Line 468: // All the partitions corresponding to the same partition path, have the same
> I don't think this is necessarily true, but you can state that we assume it
Ok, modified the comment.
Line 483: for (HdfsPartition partition: partitions) {
> This function looks much more expensive than before. We loop over all parti
Although it looks like O(N^2), it isn't. 'partitions' correspond to each partition that maps to a single location. So if we flatten List<List<Partition>> its actually all the partitions in the table. Basically we don't loop like a conventional nested loop "for each partition, for each fileStatus". We loop through the pruned partition list i.e, partitions corresponding to that path.
Line 780: Map<FsKey, FileBlocksInfo> blocksToLoad = Maps.newHashMap();
> unused?
Done
Line 784: HashMap<Path, List<HdfsPartition>> filteredParts = Maps.newHashMap();
> The var name filteredParts is a little confusing to me. How about partsByPa
Done
Line 830: if (filteredParts.containsKey(partDir)) filteredParts.get(partDir).add(partition);
> use {}
Done
Line 833: // Check if the partition directory's is a child dir of hdfsBaseDir_
> typo: directory's -> directory
Done
Line 835: !FileSystemUtil.isChildPath(partDir, getHdfsBaseDirPath())) {
> use tblLocation, that's clearer (and in comment above)
Done
Line 836: // Add the partition to the list of paths to load block MD.
> Comment only describes the code below, maybe say something like:
Done
Line 868: loadBlockMetadata(fs, location, filteredParts, fileBlocksToLoad);
> Having separate loadBlockMetadata() and loadDiskIds() doesn't seem to make
Yea it doesn't make sense. I wanted to refactor that but I forgot. Thanks for pointing it out.
Line 934: boolean isMarkedCached = isMarkedCached_;
> looks unused, are you sure nothing got broken here?
Yep, wanted to discuss about this. I'll ping you offline.
Line 953: try {
> not your change, but you can do this instead:
Done
Line 1583: boolean isMarkedCached = isMarkedCached_;
> looks unused, sure nothing is broken here?
Same as above. I wanted to discuss this offline.
Line 1628: totalHdfsBytes_ += hdfsPart.getSize();
> Why can't this stay inside addPartition()? In the loading case the size and
Yea, now that I think about it, it can be in addPartition(). Moved them.
http://gerrit.cloudera.org:8080/#/c/5148/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 347: LOG.debug("Loading block md for " + name_ + " directory " + dirPath.toString());
> Please add If(LOG.isDebugEnabled()) to avoid calling the dirPath.toString()
Done. Added it in a couple of places that can potentially spam.
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 423: if (p.isRoot()) return false;
> isn't the equals() below sufficient?
That returns true for isChildPath(root, root)?
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 19: Code-Review+2
Rebased. Carrying +2. PS 19 has passed S3 core/hdfs core/hdfs exhaustive. Running GVO.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 15: Code-Review+1
New changes lgtm, Nice catch with the useless map, Dimitris!
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 2:
(18 comments)
High-level comments. Let's get through them first and then I can dig in deeper.
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 27: * This class encapsulates the logic required for mapping HDFS
Much fluff, simplify to: Maps HDFS storage-UUIDs to per-host 0-based, sequential disk ids required by Impala backends.
Line 29: * by the backend.
required by Impala backends
Line 31: public class DiskIdMapper {
Make this a singleton to clarify the intended use.
Line 36: // This is intentionally implemented as a static object to share the storageIDs across
move this part to the class comment
Line 38: // consistent across all the tables, without which, we can potentially confuse the
also mention that the static solution requires the least amount of memory
Line 48: private static ConcurrentHashMap<String, AtomicInteger>
Don't think we need a ConcurrentHashMap or AtomicInteger here, because whenever we update this entry, we also need to protect storageIdToInt, so we need a coarser-grained lock.
Line 53: * TODO (Bharath): This method is still prone to some race conditions. Fix it before
Correct. I don't think the ConcurrentHashMaps will help us because we need to atomically update both maps sometimes.
Line 71: storageIdGenerator.put(host, new AtomicInteger(diskId));
Couple of races here like you already pointed out.
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 294: // Keeps track of all the parition paths that are filtered to add block metadata.
this comment style for classes and functions
/**
*
*/
Line 368: if (!pathMd.filteredParitionPaths.contains(fileStatus.getPath())) continue;
Looks like this contains() and the indexOf() below can become very expensive.
Instead of the 3 arrays in pathMd we should consider using a HashMap from path to a class that contains the relevant metadata (FD and format).
Line 423: for (int i=0; i < fileMd.pathFds.size(); ++i) {
i = 0 (add spaces)
Line 458: // Only build the storage IDs fs instances that support the BlockLocation API.
... for fs instances ...
Line 720: private void loadAllPartitions(
After reading through this a few times I'm still confused about the high-level code flow. It would be great to summarize the high-level steps in the function comment.
My understanding is that we do something like this:
1. createPartition() for every partition; that populates pathMd; internally createPartition() lists all files in the corresponding partition directory
2. load the block location metadata based on the contents of pathMd
The process above seems a bit backwards to me. In the first step we list all files under the partition's directory... and then we do the same thing again in step 2.
Intuitively, I was thinking it should look something like this:
1. collect all dirs to call DFS.listFiles() on
2. for each dir, call listFiles(), examine the contents and map them to an msPartition. Based on that complete info do some filtering/checks and finally createPartition().
Another chat would probably help me understand the ideas here better.
Line 777: if (dirsToLoad.contains(partDir) ||
positive is more readable to me
if (!dirsToLoad.contains() && isChildPath()) {
// Partition with a non-default data directory.
dirsToLoad.add();
}
http://gerrit.cloudera.org:8080/#/c/5148/2/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 275: * Returns true if the filesystem supports BlockLocation API.
What API is it specifically? LocatedFileStatus.getBlockLocations()?
Line 277: public static boolean supportsBlockLocationAPI(FileSystem fs) {
supportsBlockLocationApi
Line 406: public static boolean isPathChild(Path p1, Path p2) {
isChildPath(Path p, Path parent)
Line 408: Path parentDir = p1.getParent();
I think this can be improved by using the Path.getDepth(). You can do getParent() a fixed number of times and then do one equals() comparison.
(Basically you walk up the path starting at p1 until the parentDir is at the same depth as p2)
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 12:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5148/12//COMMIT_MSG
Commit Message:
Line 15: We loop throuh each and every file in the table/partition directories
> through
Done
Line 17: This results in large no. of RPC calls to namenode, especially with
> no. -> number
Done
Line 35:
> mention the behavior change of REFRESH
Done
http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 31: * - To maintain consistent mapping across all the table instances so that the disk thread
> ... a consistent mapping ... so that the assignment of scan ranges to I/O t
Done
http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 288: * Queries the filesystem to load the file block metadata (e.g. DFS blocks) for the
> Suggest rephrasing:
Done
Line 296: * and enumerate all its blocks and their corresponding hosts and disk IDs.
> remove part about disk ids, I think that's the next point
Done
Line 778: LOG.debug("partsByPath size: " + partsByPath.size());
> check log lvl
Done
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5148/9/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 38: private static DiskIdMapper INSTANCE = new DiskIdMapper();
> just make it public
Done
http://gerrit.cloudera.org:8080/#/c/5148/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 403: for (List<HdfsPartition> partLists: partsByPath.values()) {
> It seems simpler to move this logic into the while loop above and update th
Done
Line 417: loadDiskIds(perFsFileBlocks);
> I think we can get rid of this function and move the logic into the while l
Done
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 11:
(40 comments)
http://gerrit.cloudera.org:8080/#/c/5148/11/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:
Line 29: * ids. This is internally implemented as a static object to share the storageIDs across
> Several sentences Starting with "This", sometimes not clear what "This" ref
Done
Line 44: // sequential 0-based integer disk id used by the hdfs scanners. This assumes that
> used by the BE scanners.
Done
Line 46: private ConcurrentHashMap<String, Integer> storageIdToInt =
> storageUuidToDiskId
Done
Line 50: // with the corresponding latest 0-based integer ID. Given a storage UUID, we first
> with -> to
Done
Line 51: // look in 'storageIdtoInt' map if it already exists, else we generate a new ID for
> no need for this second sentence, that's what we do in getDiskId()
Done
Line 64: if (Strings.isNullOrEmpty(storageUuid)) return diskId;
> let's move this check to the caller and require the storageUuid to be non-n
Done
Line 68: // across the cluster, it is possible that we have a good hit rate.
> it is possible -> we expect to have a good hit rate.
Done
Line 72: // At this point, it could be possible due to race, that storageId might've been
> // Mapping might have been added by another thread that entered the synchro
Done
Line 76: // No mapping exists, create a new disk ID for 'storageId'
> stoprageUuid
Done
Line 80: // Host hasn't been populated yet. Start with a 0-based id.
> // First disk id of this host.
Done
http://gerrit.cloudera.org:8080/#/c/5148/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 289: * given directory 'dirPath'. Only blocks corresponding to paths from 'partsByPath'
> Mention that the block metadata is effectively dropped and re-created.
Done
Line 307: RemoteIterator<LocatedFileStatus> fileStatusIter = fs.listFiles(dirPath, true);
> move right before the while loop
Done
Line 308: // Reset the current state of partitions under dirPath from perPartitionFileDescMap_
> Reset -> Clear
Done
Line 310: for (Path partDir: partsByPath.keySet()) {
> Iterate over the entrySet(), you are using an unnecessary get(partDir) belo
Done
Line 321: String fileName = fileStatus.getPath().getName();
> move closer to where it's used, i.e. right above L336
Done
Line 322: if (!FileSystemUtil.isValidPartitionFile(fileStatus)) continue;
> isValidDataFile()
Done
Line 323: // At this point, file status points to a valid file under the directory. Now we
> // Find the partition that this file belongs (if any).
Done
Line 325: Path partPathDir = fileStatus.getPath().getParent();
> move closer to where it's used, i.e. L357
It is used in the next 2,3 lines? Moved partPathDirName to line before the loop.
Line 330: // Skip if this file doesn't correspond to any of the existing partitions.
> Skip if this file does not belong to any known partition.
Done
Line 332: LOG.debug("File " + fileStatus.getPath().toString() + " doesn't correspond " +
> add if to check log level
Done
Line 333: " to a valid partition. Skipping metadata load for this file.");
> to a known partition
Done
Line 361: Preconditions.checkState(perPartitionFileDescMap_.containsKey(partPathDirName));
> avoid duplicate lookup with get() + Preconditions.checkNotNull()
Done
Line 363: // Update the partitions metadata corresponding to this file descriptor
> partitions' metadata that this file belongs to.
Done
Line 371: LOG.warn("Unknown disk id count for filesystem " + fs + ":" + unknownDiskIdCount);
> add if to check log level
Done
Line 374: throw new RuntimeException("Error loading block md for directory "
> block metadata
Done
Line 380: * Loads the disk IDs for BlockLocation 'location' and its corresponding file block.
> Comment should mention the storage UUID to disk id mapping.
Done
Line 386: String[] hosts;
> Do you know if these are the unique hosts? I'm wondering if storageIds and
Earlier code had a if block for this. Per my understanding HDFS doesn't place more than one copy of the same block on the same machine. Still, I think its good to have it for debugging. I'm adding it with an error message.
Line 394: for (int i =0; i < storageIds.length; ++i) {
> int i = 0; (space after "=")
Done
Line 410: for (Path partPath: partsByPath.keySet()) {
> iterate over Map.Entry, you are doing a get() in the next line
Done
Line 412: // For each valid file in the partPath, synthesize the block metadata.
> For each data file in the partPath...
Done
Line 427: // We assume all the partitions corresponding to the same partition path, have
> For the purpose of synthesizing block metadata, we assume that all partitio
Done
Line 447: perPartitionFileDescMap_.put(partition.getLocation(), fileDescMap);
> move put() inside the if (fileDescMap == null) block
Done
Line 673: * Create HdfsPartition objects corresponding to 'msPartitions' and add them to this
> Briefly list the high-level steps, in particular that we first create the p
Done. Let me know if there can be any improvement.
Line 742: // Check if the partition directory is a child dir of tbl directory location.
> remove, comment only states what the code does
Done
Line 752: // Load the block metadata
> remove comment (doesn't add anything)
Done
Line 764: throw new CatalogException("Error loading block location md for partition " +
> md -> metadata
Done
Line 827: public HdfsPartition createPartition(StorageDescriptor storageDescriptor,
> change this to createAndLoadPartition()? then we can get rid of the flag in
Done
http://gerrit.cloudera.org:8080/#/c/5148/11/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 276: * Returns true if the file corresponding to 'fileStatus' is a valid file as per
> is a valid data file
Done
Line 278: * directory or a hidden file starting with . or _ or if it is a LZO index file.
> hidden file is enough, no need to mention specific prefixes
Done
Line 291: public static boolean supportsBlockLocationApi(FileSystem fs) {
> supportsBlockLocations()?
I'm fine with either. Actually BlockLocation is supported by generic FileSystem interface. So I think that isn't perfect either but wfm.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#16).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
6 files changed, 365 insertions(+), 338 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/16
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Reviewed-on: http://gerrit.cloudera.org:8080/5148
Reviewed-by: Bharath Vissapragada <bh...@cloudera.com>
Tested-by: Internal Jenkins
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
6 files changed, 367 insertions(+), 341 deletions(-)
Approvals:
Bharath Vissapragada: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#4).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use to improvement to reduce an addition
RPC to NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
3 files changed, 385 insertions(+), 250 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/4
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#12).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 380 insertions(+), 344 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/12
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 2:
Moved disk-id-mapping logic to a new class as discussed with Alex. getDiskId() still is prone to some race conditions, so I added a TODO. I just sent out the code for review while I'm working on it. That particular change is siloed and shouldn't affect rest of the review.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#6).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
3 files changed, 385 insertions(+), 250 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/6
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5148
to look at the new patch set (#17).
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.
Without this patch:
------------------
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.
With this patch:
---------------
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.
- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.
- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.
- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.
Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.
Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
6 files changed, 371 insertions(+), 338 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/17
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
......................................................................
Patch Set 4:
(6 comments)
Responding to comments. I'll wait for some of the larger changes before doing another round.
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
Line 351: Map<String, Map<String, FileDescriptor>> newFileDescMap = Maps.newHashMap();
> I did that initially but the problem happens when one of the files is delet
Before, we used to want to reuse as much of the existing block metadata as possible since that was an expensive operation. However, with this new design, we are already loading all the metadata from HDFS, so there is no need to optimize this case by not reloading the metadata (we are loading it anyway). So I think this can be simplified to just discarding the entries from the global map and updating the global map in place. We also don't need any of the "should we reload the metadata" logic.
Line 386: String[] blockHostPorts = loc.getNames();
> We are using them in preconditions check in L396 and loop condition in L403
The code and comments around these is very sparse on content, so I think we should just condense it. This method looks really long but really it is just very "fluffy", so I was hoping to make it more content dense.
Line 431: numHdfsFiles_ += fileDescs.size();
> I think its due to the current design. The reload is implemented as dropPar
Got it, makes sense.
Line 483: for (HdfsPartition partition: partitions) {
> Although it looks like O(N^2), it isn't. 'partitions' correspond to each pa
Ahh, of course. You are right. I missed the local partitions var.
Line 934: boolean isMarkedCached = isMarkedCached_;
> Yep, wanted to discuss about this. I'll ping you offline.
Looks like we used to force reloading the block locations of cached partitions. As discussed in a comment above, the optimization of only reloading the metadata for some files doesn't seem to make sense anymore, so forcing the reload based on caching doesn't make anymore, either.
http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:
Line 423: if (p.isRoot()) return false;
> That returns true for isChildPath(root, root)?
Good point. You are right.
--
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-HasComments: Yes