Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,6 @@ target/
.cache
*~
mvn_install.log
.factorypath
bin/

40 changes: 33 additions & 7 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,36 @@
language: java
before_install:
- bash dev/travis-before_install.sh
language: generic

services:
- docker

env:
- HADOOP_PROFILE=default TEST_CODECS=uncompressed,brotli
- HADOOP_PROFILE=default TEST_CODECS=gzip,snappy
global:
- JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this as an ENV step in the docker image. It is part of the docker container, instead of the Travis build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, part of the work here is to use the same image to test with Java 11 in the future if needed, and the thrift image includes Java 11. I know it looks weird but this will allow us to build two separate images.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Just to clarify, the JAVA_HOME is also set in the base image, right? https://github.com/docker-library/openjdk/blob/master/8/jdk/Dockerfile#L20

For JDK11 we're still waiting on some blockers: #596 Mostly waiting for a new release of Parquet Format apache/parquet-format#127. This enables us to update Scrooge, which is required to update to Java 11.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the parent image of this Parquet one is not openjdk, it is thrift, and that's the reason why we had to define it manually, and why I had to install manually the JDK in the scripts too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the links on Java 11 I wanted to be catch the progress of this 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor question what happened to the 1.11.0 release, i saw we are already in 1.12.0-SNAPSHOT but have not seen a release any part.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, I was under the impression that it was being built from openjdk, but thrift makes more sense. Thanks for the clarification! 👍

The 0.11 release is still in progress, but has stalled because of unsigned keys:
https://lists.apache.org/thread.html/df101f14f5b5a2489443e23be32c7342e2b8a6c8eb95f2e6953fd5b4@%3Cdev.parquet.apache.org%3E

- IMAGE=parquet-mr
- CACHE_FOLDER=$HOME/docker-images
- CACHE_FILE=${CACHE_FOLDER}/parquet-mr.tgz
matrix:
- TEST_CODECS=uncompressed,brotli
- TEST_CODECS=gzip,snappy

cache:
directories:
- "$HOME/.m2"
- ${CACHE_FOLDER}

before_install:
- if [[ -f ${CACHE_FILE} ]]; then
docker load -i "${CACHE_FILE}";
else
docker build -f dev/Dockerfile -t "${IMAGE}" "${PWD}";
mkdir -p "${CACHE_FOLDER}";
docker save "${IMAGE}" | gzip -c > "${CACHE_FILE}";
fi

install: docker run -v $HOME/.m2:/root/.m2 -v ${PWD}:/parquet-mr -e JAVA_HOME=$JAVA_HOME -e TEST_CODECS=$TEST_CODECS -i -t parquet-mr /bin/bash -c "cd /parquet-mr && mvn install --batch-mode -DskipTests=true -Dmaven.javadoc.skip=true -Dsource.skip=true | pv -fbi 60 > mvn_install.log || (cat mvn_install.log && false)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make the command part of the container as well. For example, use the WORKDIR for setting the path.

Why do we -DskipTests=true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment that above, it is better to have a more flexible container than to tight commands into it for our use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that htis is the install step and the goal here is only to put the artifcats into the .m2 directory that's the reason I found, but this code is inherited from the previous build file, as well as the one in the script step.

Copy link
Member Author

@iemejia iemejia May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely prefer to make both install and script into a single step but I wanted to stay equal until this was working and then discuss about how to improve it, e.g. the way we call the IT with an environment variable can be done as a maven failsafe execution step too, to only have one run and cover everything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks


script: docker run -v $HOME/.m2:/root/.m2 -v ${PWD}:/parquet-mr -e JAVA_HOME=$JAVA_HOME -e TEST_CODECS=$TEST_CODECS -i -t parquet-mr /bin/bash -c "cd /parquet-mr && mvn verify javadoc:javadoc"

install: mvn install --batch-mode -DskipTests=true -Dmaven.javadoc.skip=true -Dsource.skip=true | pv -fbi 60 > mvn_install.log || (cat mvn_install.log && false)
script: mvn verify javadoc:javadoc -P $HADOOP_PROFILE
before_cache:
- rm -Rf $HOME/.m2/repository/org/apache/parquet
- find $HOME/.m2/repository/ -name *SNAPSHOT | xargs rm -Rf
28 changes: 28 additions & 0 deletions dev/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
###############################################################################
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
###############################################################################

FROM thrift:0.12.0

RUN set -ex; \
apt-get update && \
apt-get install -y --no-install-recommends \
maven \
openjdk-8-jdk-headless \
openjdk-8-jre-headless \
pv && \
rm -rf /var/lib/apt/lists/*
50 changes: 0 additions & 50 deletions dev/travis-before_install-master.sh

This file was deleted.

44 changes: 0 additions & 44 deletions dev/travis-before_install.sh

This file was deleted.

1 change: 1 addition & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@
<exclude>**/*.md.vm</exclude>
<exclude>**/.classpath</exclude>
<exclude>**/.project</exclude>
<exclude>**/*.factorypath</exclude>
<exclude>**/.settings/**</exclude>
<exclude>**/build/**</exclude>
<exclude>**/target/**</exclude>
Expand Down