Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 )
Change subject: IMPALA-11478: Cleanup JniCatalog ...................................................................... Patch Set 5: Code-Review+1 (3 comments) Looks good to me, just some extra cleanup / redundancy reduction ideas. http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@20 PS4, Line 20: import static org.apache.impala.service.JniCatalogOp.execOp; Using a static function from other class without prefixing with the class name looks a bit unusual for me. A simple private wrapper function could be created in JniCatalog that calls to JniCatalogOp.execOp() if the goal is the make the code shorter by avoiding adding JniCatalog. at every call. Having such wrapper member function could also reach members like protocolFactory_ if needed. http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@252 PS4, Line 252: byte[] res = serializer.serialize(catalogOpExecutor_.execDdlRequest(params)); : return Pair.create(res, (long) res.length); This looks like a very common pattern, an alternative of execOp could avoid the boilerplate code by expecting a TSerializer an extra param while the lambda would return TSerializable. So this could look like: return execOp2("execDdl", shortDesc, () -> { return catalogOpExecutor_.execDdlRequest(params)); }, params. serializer); http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@265 PS4, Line 265: catalogOperationUsage If already cleaning up this could get a trailing _ (it confused me for a bit first). -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 06 Dec 2022 15:13:06 +0000 Gerrit-HasComments: Yes
