Skip to content

Fix quickstart doc with docker compose #1610

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.

Sign up for GitHub

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

Merged
flyrain merged 9 commits into from
May 23, 2025
Merged

Fix quickstart doc with docker compose #1610

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c125b2b
Fix quickstart doc with docker compose
MonkeyCanCode May 19, 2025
6a4be53
Fix quickstart doc with docker compose
MonkeyCanCode May 20, 2025
f776509
Fix quickstart doc with docker compose
MonkeyCanCode May 20, 2025
aa8dc9f
Merge branch 'main' into fix_quickstart_doc
MonkeyCanCode May 21, 2025
1c4a0b8
Fix quickstart doc with docker compose
MonkeyCanCode May 21, 2025
37b35eb
Fix quickstart doc with docker compose
MonkeyCanCode May 21, 2025
841ec2e
Fix quickstart doc with docker compose
MonkeyCanCode May 23, 2025
f338738
Fix quickstart doc with docker compose
MonkeyCanCode May 23, 2025
8fd5ff8
Fix conflict
MonkeyCanCode May 23, 2025
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
2 changes: 2 additions & 0 deletions getting-started/assets/.env
View file
Open in desktop
Comment thread
MonkeyCanCode marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CLIENT_ID=root
CLIENT_SECRET=s3cr3t
2 changes: 1 addition & 1 deletion getting-started/assets/postgres/docker-compose-postgres.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ services:
volumes:
# Bind local conf file to a convenient location in the container
- type: bind
source: ../assets/postgres/postgresql.conf
source: ${ASSETS_PATH}/postgres/postgresql.conf
target: /etc/postgresql/postgresql.conf
command:
- "postgres"
Expand Down
3 changes: 2 additions & 1 deletion getting-started/eclipselink/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ This example requires `jq` to be installed on your machine.
2. Start the docker compose group by running the following command from the root of the repository:

```shell
docker compose -f getting-started/eclipselink/docker-compose-bootstrap-db.yml -f getting-started/assets/postgres/docker-compose-postgres.yml -f getting-started/eclipselink/docker-compose.yml up
export ASSETS_PATH=$(pwd)/getting-started/assets/
docker compose --env-file getting-started/assets/.env -f getting-started/assets/postgres/docker-compose-postgres.yml -f getting-started/eclipselink/docker-compose-bootstrap-db.yml -f getting-started/eclipselink/docker-compose.yml up
```

3. Using spark-sql: attach to the running spark-sql container:
Expand Down
4 changes: 2 additions & 2 deletions getting-started/eclipselink/docker-compose-bootstrap-db.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ services:
polaris.persistence.type: eclipse-link
polaris.persistence.eclipselink.configuration-file: /deployments/config/eclipselink/persistence.xml
volumes:
- ../assets/eclipselink/:/deployments/config/eclipselink
- ${ASSETS_PATH}/eclipselink/:/deployments/config/eclipselink
command:
- "bootstrap"
- "--realm=POLARIS"
- "--credential=POLARIS,root,s3cr3t"
- "--credential=POLARIS,${CLIENT_ID},${CLIENT_SECRET}"
polaris:
depends_on:
polaris-bootstrap:
Expand Down
8 changes: 4 additions & 4 deletions getting-started/eclipselink/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ services:
quarkus.otel.sdk.disabled: "true"
POLARIS_BOOTSTRAP_CREDENTIALS: POLARIS,${CLIENT_ID},${CLIENT_SECRET}
volumes:
- ../assets/eclipselink/:/deployments/config/eclipselink
- ${ASSETS_PATH}/eclipselink/:/deployments/config/eclipselink
healthcheck:
test: ["CMD", "curl", "http://localhost:8182/q/health"]
interval: 2s
Expand All @@ -58,7 +58,7 @@ services:
- CLIENT_ID=${CLIENT_ID}
- CLIENT_SECRET=${CLIENT_SECRET}
volumes:
- ../assets/polaris/:/polaris
- ${ASSETS_PATH}/polaris/:/polaris
entrypoint: '/bin/sh -c "chmod +x /polaris/create-catalog.sh && /polaris/create-catalog.sh"'

spark-sql:
Expand All @@ -82,7 +82,7 @@ services:
--conf, "spark.sql.catalog.quickstart_catalog.type=rest",
--conf, "spark.sql.catalog.quickstart_catalog.warehouse=quickstart_catalog",
--conf, "spark.sql.catalog.quickstart_catalog.uri=http://polaris:8181/api/catalog",
--conf, "spark.sql.catalog.quickstart_catalog.credential=${USER_CLIENT_ID}:${USER_CLIENT_SECRET}",
--conf, "spark.sql.catalog.quickstart_catalog.credential=${CLIENT_ID}:${CLIENT_SECRET}",

@adnanhemani adnanhemani May 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a regression, it should remain USER_CLIENT_ID/SECRET - these are user credentials rather than the admin credentials in CLIENT_ID/SECRET

@MonkeyCanCode MonkeyCanCode May 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did noticed this one last night and assuming there is intensional. However, going through the quickstart guide, this is supposed to be the credential created at the end of the polaris CLI. But USER_CLIENT_ID and USER_CLIENT_SECRET is never defined in this context I believed. As an end-user tries to bring up setup via docker compose, there is no relation to the polaris CLI.

Now assuming we want to keep it this way, an end-user can't really define this one until polaris CLI returns the value back (which needed to bring up docker compose first, run polaris CLI for entities creation, then restart spark-sql to have this be effective). This restart workflow is really odd imo and we should replace it with other more automated workflow.

If this is causing regression, I can change it back. But I don't think we ask user to follow the restart workflow as it is really tedious imo.

Let me know what you think.

@adnanhemani adnanhemani May 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Take a look at this: https://polaris.apache.org/in-dev/unreleased/getting-started/using-polaris/#connecting-with-spark

We do ask users to refresh their Docker containers once they export the required variables.

You've correctly pointed out the chicken-and-the-egg problem we have here, but unfortunately this is how we have solved it so far - asking the user for one line ran is not completely unreasonable and we have no better known solution without breaking the Docker Compose file down even further :(

@MonkeyCanCode MonkeyCanCode May 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But there are couple problem with both existed route as well as the above, let me share my observations:

On terminal 1, we will run the following:

export ASSETS_PATH=$(pwd)/getting-started/assets/
docker compose --env-file getting-started/assets/getting-started.env \
  -f getting-started/assets/postgres/docker-compose-postgres.yml \
  -f getting-started/eclipselink/docker-compose-bootstrap-db.yml \
  -f getting-started/eclipselink/docker-compose.yml up

Due to USER_CLIENT_ID and USER_CLIENT_SECRET are not yet defined at this stage, the spark-sql will fail due to fail auth (as both will resolve to empty string). So the container is in stop state.

Now on terminal 2, we do the needed work to bootstrap polaris entities and have a different principal created and run the following to set values for environment variables:

export USER_CLIENT_ID=xxxxx
export USER_CLIENT_SECRET=xxxx

and try to start the failed containers here with current command in the doc:

docker compose -f getting-started/eclipselink/docker-compose.yml up -d

This will actually crashed the first setup as two docker compose command on diff terminal/session will create two diff projects and they are trying to bind to the same ports.

Now to workaround the problem, I added -p xxx to specify project so two commands from diff sessions will be dealing with same resources. However, this doesn't solve the problem of created container doesn't reload (which back to my original point where this is not really a regression as it never works before). To have containers reload the env, we will need to stop/remove the container then recreate it. By doing so, it will then try to load envs from the client's terminal.

@adnanhemani adnanhemani May 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, missed this comment. But I see what's the issue here - the original Docker Compose command should've been run with the "-d" detached option so that everything runs in one terminal only. Not sure how I missed that, sorry about that. We shouldn't need to force users to run multiple terminals for any of these commands.

--conf, "spark.sql.catalog.quickstart_catalog.scope=PRINCIPAL_ROLE:ALL",
--conf, "spark.sql.defaultCatalog=quickstart_catalog",
--conf, "spark.sql.catalogImplementation=in-memory",
Expand All @@ -102,4 +102,4 @@ services:
ports:
- "8080:8080"
volumes:
- ../assets/trino-config/catalog:/etc/trino/catalog
- ${ASSETS_PATH}/trino-config/catalog:/etc/trino/catalog
3 changes: 2 additions & 1 deletion site/content/in-dev/unreleased/getting-started/quickstart.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ cd ~/polaris
:polaris-quarkus-admin:assemble --rerun \
-Dquarkus.container-image.tag=postgres-latest \
-Dquarkus.container-image.build=true
docker compose -f getting-started/eclipselink/docker-compose-postgres.yml -f getting-started/eclipselink/docker-compose-bootstrap-db.yml -f getting-started/eclipselink/docker-compose.yml up
export ASSETS_PATH=$(pwd)/getting-started/assets/

@adnanhemani adnanhemani May 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move this line to the top of the file with the exporting of the CLIENT_ID/SECRET, so that it only needs to be run once?

@MonkeyCanCode MonkeyCanCode May 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I followed, this is the beginning of the quickstart page (https://polaris.apache.org/in-dev/unreleased/getting-started/quickstart/#docker-image). This is only needed for the context of docker compose.

The export of CLIENT_ID/CLIENT_SECRET is invalid I think as the current state (without the env) file, this won't even be able to start.

If I understand correctly, we should consider move export of CLIENT_ID/CLIENT_SECRET to this section (as the current docker compose file has no credential, so it will try to set empty string for root credential (as well as username, which is in-valid).

The export is only needed if user doesn't want to use env file (as env file will load the credential in the updated command). Let me know what you think.

@adnanhemani adnanhemani May 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, this comment is in conjunction with the suggestion from the overall review's comment. We should do the following:

  1. Move the export of CLIENT_ID/CLIENT_SECRET to the top of the file - the Docker Compose files will be able to intake the environment variables set in bash. (i.e. keep one reference at the top of the Quickstart page and one at the top of each of the cloud deployment pages)
  2. Remove all references to setting the CLIENT_ID/CLIENT_SECRET elsewhere.
  3. Add the export ASSETS_PATH to these references to setting CLIENT_ID/CLIENT_SECRET

Does this make sense?

@MonkeyCanCode MonkeyCanCode May 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I got what you mean. I had made some changes to this doc for refactor. Please review.

docker compose --env-file getting-started/assets/.env -f getting-started/assets/postgres/docker-compose-postgres.yml -f getting-started/eclipselink/docker-compose-bootstrap-db.yml -f getting-started/eclipselink/docker-compose.yml up
```

You should see output for some time as Polaris, Spark, and Trino build and start up. Eventually, you won’t see any more logs and see some logs relating to Spark, resembling the following:
Expand Down