Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14964 )
Change subject: KUDU-3007. Support building Kudu on aarch64 platform ...................................................................... Patch Set 18: (10 comments) Overall looks good, a few nits at this point. http://gerrit.cloudera.org:8080/#/c/14964/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14964/18//COMMIT_MSG@9 PS18, Line 9: This change try to modify and make Kudu can build sucessfully on both : x86_64 and aarch64 platform. How about rephrasing it a bit: With this patch, Kudu can be built and run on both x86_64 and aarch64 platforms. http://gerrit.cloudera.org:8080/#/c/14964/18/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/14964/18/build-support/jenkins/build-and-test.sh@207 PS18, Line 207: all of test cases failed all tests fail http://gerrit.cloudera.org:8080/#/c/14964/18/build-support/jenkins/build-and-test.sh@208 PS18, Line 208: so apply static linking direcly rephrase for clarity: ..., so use static linking. http://gerrit.cloudera.org:8080/#/c/14964/18/java/kudu-client/build.gradle File java/kudu-client/build.gradle: http://gerrit.cloudera.org:8080/#/c/14964/18/java/kudu-client/build.gradle@39 PS18, Line 39: testCompile project(":kudu-hive") Why is necessary to compile it in case of aarch64? If it's really needed, could you add a comment explaining the reasoning? http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/columnar_serialization.cc File src/kudu/common/columnar_serialization.cc: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/columnar_serialization.cc@22 PS18, Line 22: //__aarch64__ nit: drop this comment, it doesn't add much value and even a bit confusing, IMHO http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/key_encoder.h File src/kudu/common/key_encoder.h: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/key_encoder.h@286 PS18, Line 286: #else : return false; : #endif //__aarch64__ nit: from the readability perspective, I think it will be better to have this in the beginning. I.e. #ifdef __aarch64__ return false; #else ... #endif http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/atomicops-internals-x86.cc File src/kudu/gutil/atomicops-internals-x86.cc: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/atomicops-internals-x86.cc@25 PS18, Line 25: #if defined(__i386__) || defined(__x86_64__) Instead, is it possible to exclude this file from compilation at cmake level, updating src/kudu/gutil/CMakeLists.txt ? That can make compilation a bit faster, avoiding empty object files. http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/dynamic_annotations.h File src/kudu/gutil/dynamic_annotations.h: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/dynamic_annotations.h@60 PS18, Line 60: #include <stddef.h> nit: consider instead #include <cstddef> http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/util/init.cc File src/kudu/util/init.cc: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/util/init.cc@69 PS18, Line 69: == nit: add spaces before and after '=='? http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/util/init.cc@69 PS18, Line 69: if (!cpu.has_broken_neon() && cpu.cpu_brand()=="ARM64") { Could you add a comment explaining -- To view, visit http://gerrit.cloudera.org:8080/14964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2953519c5d28de17e6b2bb7094abab0c1cd12c97 Gerrit-Change-Number: 14964 Gerrit-PatchSet: 18 Gerrit-Owner: liusheng <[email protected]> Gerrit-Reviewer: Adar Lieber-Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: RuiChen <[email protected]> Gerrit-Reviewer: liusheng <[email protected]> Gerrit-Comment-Date: Tue, 28 Apr 2020 20:04:20 +0000 Gerrit-HasComments: Yes
