Henry Robinson has posted comments on this change. Change subject: IMPALA-1850 allow fs.defaultFS to be set to a non-HDFS filesystem ......................................................................
Patch Set 10: Code-Review+1 (6 comments) This looks good to me for now. As I understand it, we have lost a small amount of coverage for tests with multiple filesystems (I'm not sure when those would ever get run - maybe with LocalFS?). Let's file a JIRA to have some test setup where we exploit multiple filesystems. It would be much better to config Impala as follows: DEFAULT_FS=S3 SECONDARY_FS=HDFS . bin/impala-config.sh and have that set the system configuration up as expected. http://gerrit.cloudera.org:8080/#/c/1121/10//COMMIT_MSG Commit Message: Line 7: IMPALA-1850 allow fs.defaultFS to be set to a non-HDFS filesystem nit: IMPALA-1850: Allow fs.defaultFS... Line 9: . What are the supported filesystems? I think you mean supported filesystem *as default FS* which is different. Line 9: impala Impala Line 10: S3 I think this is better phrased as "This patch configures Impala to use S3 as the default filesystem, rather than a secondary filesystem as before." Line 10: remove extra space http://gerrit.cloudera.org:8080/#/c/1121/10/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: Line 687: ( you can remove these parentheses: if (!(fs instanceof DFS || fs instanceof S3A)) { -- To view, visit http://gerrit.cloudera.org:8080/1121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2f45bef6c94ece634045acb906d12591587ccfed Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: [email protected] Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes
