Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for docker image with presto java #7920

Closed
wants to merge 7 commits into from

Conversation

kgpai
Copy link
Contributor

@kgpai kgpai commented Dec 7, 2023

This image will allow us to run Velox + Presto java and allow Velox to use Presto java as a source of truth for its fuzzer runs.

Once the image is built , you can run presto java on it by calling /opt/start-prestojava.sh

Copy link

netlify bot commented Dec 7, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 027de80
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65726ccc1919790008dee478

@kgpai kgpai requested a review from assignUser December 7, 2023 22:47
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 7, 2023
@kgpai kgpai requested a review from kagamiori December 7, 2023 22:47
@kgpai kgpai force-pushed the add_prestojava_image branch 4 times, most recently from 44b450c to a27a51c Compare December 7, 2023 23:38
@kgpai kgpai force-pushed the add_prestojava_image branch from a27a51c to f956dab Compare December 7, 2023 23:45
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Could you apply the following diff, it will speed up the docker workflow by running each docker build on a seperate runner, that way one image failing doesn't prevent the others from going through. (applied diff directly)

docker-compose.yml Show resolved Hide resolved
# or
# docker-compose run -e NUM_THREADS=<NUMBER_OF_THREADS_TO_USE> --rm presto-java
# to set the number of threads used during compilation
image: ghcr.io/facebookincubator/velox-dev:amd64-centos-8-avx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
image: ghcr.io/facebookincubator/velox-dev:amd64-centos-8-avx
image: ghcr.io/facebookincubator/velox-dev:presto-java

This will be the name of the build image :)

@@ -38,7 +38,33 @@ permissions:

jobs:
linux:
name: "Build and Push ${{ matrix.name }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to @assignUser .

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@kgpai kgpai requested a review from pedroerp December 8, 2023 17:06
@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in d52d68b.

facebook-github-bot pushed a commit that referenced this pull request Jan 16, 2024
…tal jobs. (#8314)

Summary:
**What ?**
  Recently support for [PrestoQueryRunner ](#7628 added to AggregationFuzzer, so that it can use Presto as a source of truth.  This PR sets up an experimental job that uses the Presto Java container to run Aggregation Fuzzer with Presto as source of truth.

**Why ?**
  Running the aggregation fuzzer against Presto gives us greater confidence on the correctness of our aggregate functions.

**How ?**
   1. [Previously we created a docker image with support for java and Presto](#7920)
   2. This still requires some changes to get Presto to work with AggregationFuzzer. These changes are listed below.
   3. Adds support for Hive metastore - we do this via a file based metastore  by adding the hive.properties file to $PRESTO_HOME/etc/catalog (see #8111)
   4. We also set the pid file on the launcher to ensure there is only one run of the Presto server
   5. Another thing to note is that we set the timezone to be ['Americas/Los_Angeles' due to details described here ](#8127).

**Testing:**

There is no automated way to test this. Here are manual steps I follow:

1. Create presto-java-container by running `docker-compose build presto-java`
6. Ssh into the presto-java-container
7. Copy Velox sources into the presto-java-container and build Velox
8. Launch Presto service: /opt/start-prestojava.sh
9. Run Aggregation fuzzer with --presto_url=http://127.0.0.1:8080 flag

Example Run: Here is an example run https://github.com/facebookincubator/velox/actions/runs/7508139395/job/20443035814

Pull Request resolved: #8314

Reviewed By: kagamiori

Differential Revision: D52701237

Pulled By: kgpai

fbshipit-source-id: e4ba284aed469bc69cc95c90f708a0c92a040205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants