Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15072 )
Change subject: IMPALA-9265: Support for toolchain Kudu to provide Java artifacts ...................................................................... Patch Set 2: (3 comments) I think the changes to the docker side make sense. I have a few small comments, but it sounds good. For the actual kudu build steps, I think we'll need to see what Impala needs from the Kudu java side to get this right. I left a comment about what I think we would need, but I don't think our progress needs to be blocked on everything being perfect. One way forward is that we put the Kudu java artifacts in a subdirectory (which we know will not conflict with the existing implementation) and then it becomes fairly harmless to check in something that is not perfect. Then, as we do the Impala change and find what we need, we do an additional change to get anything we missed. Another way forward is to merge the docker stuff (which lets us update the docker images) and then do the Kudu part in concert with the Impala change. http://gerrit.cloudera.org:8080/#/c/15072/2/docker/redhat6.df File docker/redhat6.df: http://gerrit.cloudera.org:8080/#/c/15072/2/docker/redhat6.df@15 PS2, Line 15: # Install a newer java-1.8.0-openjdk-devel from centos:6.8. : # The java-1.8.0-openjdk-devel version shipped with centos:6.6 is unable to handle ECDHE : # ciphers. : RUN yum-install --disablerepo='*' --enablerepo=C6.8-base java-1.8.0-openjdk-devel The way I think about this is that there are some libraries we use newer versions for because we need that to even run yum (or apt or whatever). Those obviously get installed before the normal command, because we have no choice. For openjdk, we have the choice of installing it before or after the command below that installs everything else. Either could potentially work. I prefer to install the newer openjdk after the command that installs everything else. That means that we use as much from the older repo as possible, because the packages below will get all their dependencies from the old repo. So, please move this below the big install command. Let me know if that starts to fail. http://gerrit.cloudera.org:8080/#/c/15072/2/docker/redhat7.df File docker/redhat7.df: http://gerrit.cloudera.org:8080/#/c/15072/2/docker/redhat7.df@9 PS2, Line 9: # We get a newer java-1.8.0-openjdk-devel from centos:7.4. : # The java-1.8.0-openjdk version shipped with centos:7.2 is unable to handle ECDHE : # ciphers. : RUN yum-install --disablerepo='*' --enablerepo=C7.4-base java-1.8.0-openjdk-devel Same as the redhat6.df comment, move this below the big install command. http://gerrit.cloudera.org:8080/#/c/15072/2/source/kudu/build.sh File source/kudu/build.sh: http://gerrit.cloudera.org:8080/#/c/15072/2/source/kudu/build.sh@140 PS2, Line 140: wrap ./gradlew :kudu-hive:assemble : for F in kudu-hive/build/libs/kudu-*.jar; do : cp "$F" "$JAVA_INSTALL_DIR" : done My thought on this part is that we are going to want to test it with the corresponding Impala change. My understanding is that there are two parts we need from the Kudu side. The first is the kudu-hive jars, which we use for the hms integration. I think those are mainly needed for tests. The second is the kudu client dependency for frontend compilation: https://github.com/apache/impala/blob/master/fe/pom.xml#L390-L394 For that, we need a valid maven repository that we can use to get the kudu client (and potentially its dependencies). So, here are some comments for now: - Impala is going to want the kudu-client in a valid maven repository, which has a particular structure. Look in your ~/.m2 directory to see the structure. We'll want a command that generates that structure. - I think we will want these jars/maven repo to be in a subdirectory of $LOCAL_INSTALL. Right now, we have $LOCAL_INSTALL/debug and $LOCAL_INSTALL/release. Maybe we should put the java artifacts / maven repo in a $LOCAL_INSTALL/java directory (or even separate directories for kudu-hive vs the maven repo). We will be able to set the IMPALA_KUDU_JAVA_HOME variable to this directory. -- To view, visit http://gerrit.cloudera.org:8080/15072 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba03dfe9c302513b825cbed7146c582e7d97c3af Gerrit-Change-Number: 15072 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Comment-Date: Wed, 22 Jan 2020 18:35:31 +0000 Gerrit-HasComments: Yes
