Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19324 )
Change subject: IMPALA-11476: Support Ozone erasure coding ...................................................................... Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/19324/11/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: http://gerrit.cloudera.org:8080/#/c/19324/11/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@233 PS11, Line 233: if (fs instanceof DistributedFileSystem) { > It is, although we'll need a specialization for isOfsFileSystem or still ha We switched to using the scheme in IMPALA-10266. https://github.com/apache/impala/commit/4b5c66f329cdd818dd11cd1a9c68b58c84bcf45c https://issues.apache.org/jira/browse/IMPALA-10266 My main thought here is that it would be nice to stick with the scheme if we can. The concern would be that class names can change. That is less of a concern with HDFS and Ozone, because we have tests and would probably notice. So, I guess this is more of a consistency thing. http://gerrit.cloudera.org:8080/#/c/19324/11/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@316 PS11, Line 316: switch (tokens.countTokens()) { : case 0: : return Pair.create("", ""); : case 1: : return Pair.create(tokens.nextToken(), ""); : default: : return Pair.create(tokens.nextToken(), tokens.nextToken()); : } > Sure. I don't really need to rewrite this, just noticed a StringTokenizer w Refactoring is fine. My nit is that I find multiple arguments of tokens.nextToken() to make me think a bit about order of evaluation. We have unit tests and it must be correct, but using local variables would make it very obvious. -- To view, visit http://gerrit.cloudera.org:8080/19324 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I201e2e33ce94bbc1e81631a0a315884bcc8047d1 Gerrit-Change-Number: 19324 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 24 Jan 2023 20:49:12 +0000 Gerrit-HasComments: Yes
