Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4847: Simplify HdfsTable block metadata loading code
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7652/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

PS1, Line 252: for all partitions in 'partitions'
> nit: "of all the 'partitions' that ..."
Done


Line 258:    *   and disk IDs.
> Expand this to mention that we synthesize block metadata for FS that don't 
Done


PS1, Line 331: if (partitions == null || partitions.isEmpty()) return;
             :     RemoteIterator<LocatedFileStatus> fileStatusIter =
             :         FileSystemUtil.listFiles(fs, partDir, false);
             :     if (fileStatusIter == null) return;
             :     while (fileStatusIter.hasNext()) {
             :       LocatedFileStatus fileStatus = fileStatusIter.next();
             :       if (!FileSystemUtil.isValidDataFile(fileStatus)) continue;
             :       // For the purpose of synthesizing block metadata, we 
assume that all partitions
             :       // with the same location have the same file format.
             :       FileDescriptor fd = 
FileDescriptor.createWithSynthesizedBlockMd(fileStatus,
             :           partitions.get(0).getFileFormat(), hostIndex_);
             :       // Update the partitions' metadata that this file belongs 
to.
             :       for (HdfsPartition partition: partitions) {
             :         partition.getFileDescriptors().add(fd);
             :       }
             :     }
> With the exception of L340, this function resembles loadBlockMetadata. Mayb
Did some refactoring. No more synthesize* methods in HdfsTable now. LMK if it 
looks ok.


PS1, Line 581: Path tblLocation = 
FileSystemUtil.createFullyQualifiedPath(getHdfsBaseDirPath());
> It looks like this is only used in L590. Maybe move it closer to that.
Done


PS1, Line 695: .keys()
> remove
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7652
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I963d647bd2ba11e3843c6ef2ac6be113c74280bf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to