Fix quickstart doc with docker compose #1610

MonkeyCanCode · 2025-05-19T05:38:31Z

This PR fixes couple issues found while trying to go through quickstart guide:

Doc site/content/in-dev/unreleased/getting-started/quickstart.md has an invalid ref to getting-started/eclipselink/docker-compose-postgres.yml. This is updated to getting-started/assets/postgres/docker-compose-postgres.yml (this is correct in getting-started/eclipselink/README.md but not in the public page)
Due to now we have 3 docker compose and 1 in a diff path, running this command can cause failure as they 2 of them have binds from local to container but from diff paths (it will create empty dirs on the wrong path). To fix this, I introduced ASSETS_PATH to point to the absolute path of assets directory which is being used for binding.
Doc getting-started/eclipselink/docker-compose-bootstrap-db.yml has a hard-coded value of default username/password which is no longer valid after recent change in Use env var in spark container #1522
quickstart page (https://polaris.apache.org/in-dev/unreleased/getting-started/quickstart/#docker-image) ask user to build the image then run docker compose and asks user to do expose CLIENT_ID and CLIENT_SECRET. Due to change in item 3, that is no longer valid as default value are not longer available. This will cause failure as well. To fix this, I add .env file to contain the original default value. This will continue to allow user to set them to diff by remove env loading for docker compose.

pingtimeout

+1

This PR raises a question about how we structure the docker-compose commands. I understand the desire to reduce the amount of duplicated yaml across docker-compose files. However, for the sake of being beginner-friendly, I would rather have everything contained in a single docker-compose file, even if it slightly increases the maintenance effort (think upgrading the Postgres version). Wdyt?

getting-started/assets/.env

dimas-b · 2025-05-19T19:33:18Z

I would rather have everything contained in a single docker-compose file

I support that. I even suggested that in #1470 (comment) :)

adnanhemani · 2025-05-19T21:24:35Z

While there are some good call-outs in this PR of things we should change, there are also some regressions. Please do not merge this PR - give me until EOD to get comments in.

adnanhemani

@MonkeyCanCode - lots of great catches here. #1522 introduced a major new change in Quickstart where we are no longer relying on default credential values and must define them up front - and we did not change the website to fully reflect that. For allowing this change and not recognizing the regression to the documentation, I apologize.

We had discussed an ".env" file in this thread but ultimately decided against it: #1522 (comment). I still agree with the comments posted there. I think that if we move this section into the top of the Quickstart page and then remove the later instructions to export the same environment variables, it would be best. We should do the same for the Cloud Provider Deployment instructions and the READMEs in the Getting Started folder.

100% agreed on the ASSETS_PATH change and the correcting of the YAML file locations.

adnanhemani · 2025-05-20T01:35:25Z

getting-started/eclipselink/docker-compose.yml

@@ -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}",


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

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.

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 :(

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.

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.

adnanhemani · 2025-05-20T01:36:09Z

site/content/in-dev/unreleased/getting-started/quickstart.md

@@ -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/


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?

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.

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

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)

Remove all references to setting the CLIENT_ID/CLIENT_SECRET elsewhere.

Add the export ASSETS_PATH to these references to setting CLIENT_ID/CLIENT_SECRET

Does this make sense?

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

adnanhemani · 2025-05-20T01:38:53Z

This PR raises a question about how we structure the docker-compose commands. I understand the desire to reduce the amount of duplicated yaml across docker-compose files. However, for the sake of being beginner-friendly, I would rather have everything contained in a single docker-compose file, even if it slightly increases the maintenance effort (think upgrading the Postgres version). Wdyt?

I disagree on this - if you read my reply to the comment that @dimas-b linked (#1470 (comment)), there's the reason why the Dockerfiles need to stay piecewise (tldr - it doesn't have to do with duplicated YAML but with the ability to reload services when required). If you have a better way to solve that issue, I'm open to it!

MonkeyCanCode · 2025-05-21T06:04:09Z

I would rather have everything contained in a single docker-compose file

I support that. I even suggested that in #1470 (comment) :)

If we all want to do it in a single docker compose file, I can take care this in the next PR. Both route works and there are ways around to make them work. More toward to a preference things (e.g. a single giant compose file may be hard to read and maybe we will have other service such as minio which we don't want to start out of box...but an end-user can decide if minio should be spin up by adding additional compose file to the command).

adnanhemani

Really close - last few comments

adnanhemani · 2025-05-21T18:52:41Z

site/content/in-dev/unreleased/getting-started/quickstart.md

+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.


I think this is introducing additional mental complexity - why don't we just require everyone to export these variables and then remove the .env file itself?

Additionally, we need to do the same thing to the Cloud Providers pages - should be a simple copy-paste change to all 3 pages.

I think env file is helping. Currently we have auth hard-coded in couple place such as auth info for Postgres. Later on if we want to integrate with additional services (e.g. minio/keycloak), we will need to do the same with current route. Introducing a env file is pretty helpful for having a centralized location for config/auth info imo and keep the docker compose file clean. It is not a must for this PR, but I do think it will avoid many of those issues in the long run.

Then for cloud providers piece, I think those are working on https://github.com/apache/polaris/pull/1646/files?

I personally think that we shouldn't introduce these further services directly into the Quickstart flow that we publish on the website - and so the environment file wouldn't be helpful even in the future. Those integrations will need to write their own Docker Compose files and all. But I can see your point as well. I'm not going to hold a hard opinion on this as a result.

Cloud Providers weren't covered in #1646. I do think that piece is a requirement for this PR though, we should ensure changes are being made together for both local and cloud providers, in general, or we risk getting to the point where things are broken again as they are now. Thoughts?

Fine with me. The only problem is I don't have a way to test the cloud deployment (so I am assuming it should work...PR is updated). There are more issues with those inconsistencies. Lets get this merge first then we can fix other issues as the existed one is not even use-able. WDYT?

Oh, the variables need to be exported prior to the bash script being run in the cloud deployments. I can re-test later tomorrow! Or worst-case, I can make the change myself tomorrow as well - not a big issue.

flyrain · 2025-05-23T05:25:51Z

site/content/in-dev/unreleased/getting-started/quickstart.md

-You can also set these credentials as environment variables for use with the Polaris CLI:
-```shell
-export CLIENT_ID=root
-export CLIENT_SECRET=secret
-```


I think we still need this to enable polaris cli.

flyrain · 2025-05-23T05:26:37Z

site/content/in-dev/unreleased/getting-started/quickstart.md

@@ -83,11 +89,6 @@ When using a Gradle-launched Polaris instance in this tutorial, we'll launch an
 For more information on how to configure Polaris for production usage, see the [docs]({{% relref "../configuring-polaris-for-production" %}}).

 When Polaris is run using the `./gradlew run` command, the root principal credentials are `root` and `secret` for the `CLIENT_ID` and `CLIENT_SECRET`, respectively.


Not a blocker: We could actually remove this line.

flyrain

Thanks @MonkeyCanCode for working on it. LGTM. Left minor comments.

flyrain · 2025-05-23T05:36:45Z

site/content/in-dev/unreleased/getting-started/using-polaris.md

+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml stop trino
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml rm -f trino
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml up -d --no-deps trino


I'd prefer to provide jdbc examples instead of eclipseLink ones here, we could do that in a follow-up PR though.

flyrain · 2025-05-23T05:37:05Z

site/content/in-dev/unreleased/getting-started/using-polaris.md

+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml stop spark-sql
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml rm -f spark-sql
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml up -d --no-deps spark-sql


Same here, prefer to have a JDBC example.

flyrain · 2025-05-23T05:44:40Z

Thanks @MonkeyCanCode for the fix! Thanks @pingtimeout , @dimas-b , and @adnanhemani for the review. Let's keep fixing what's left.

* fix(nightly-CI): Do not publish snapshots from forks (apache#1635) Adopt the `Nightly Build` workflow to not (try to) publish every night from forks. * main: Update dependency io.smallrye.config:smallrye-config-core to v3.13.0 (apache#1637) * Use echo to print script errors (apache#1648) * [HOTFIX] QUICKSTART (apache#1646) The change adds the following to fix Quick start experience : [1] ENV variables required by common assets after apache#1522 [2] New configs required to enable FILE based sources apache#1649 Co-authored-by: singhpk234 <singhpk234@users.noreply.github.com> Co-authored-by: pjanuario <pjanuario@users.noreply.github.com> * main: Update dependency gradle to v8.14.1 (apache#1652) * main: Update dependency gradle to v8.14.1 * Re-adopt PR to the project's needs --------- Co-authored-by: Robert Stupp <snazy@snazy.de> * [Policy Store] Add policyTypeCode to Slice/Index for Future Filtering Support and Update Policy Persistence Method (apache#1628) This PR adds policyTypeCode to the in-memory tree map store's slice and the SQL index on policy_mapping_records (already done in JDBC in apache#1468). This prepares for future features that need to filter efficiently by policy type, like listing all entities with a data compaction policy. It also updates the loadAllTargetsOnPolicy method to accept policyTypeCode, enabling it to use the new index for better performance. * fix(test): Do not let some more tests spam `/tmp` (apache#1651) * fix(test): Do not let some more tests not spam `/tmp` * `PolarisRestCatalogViewFileIntegrationTest` * `FileIOExceptionsTest` * `PolarisRestCatalogViewFileIntegrationTest` Changes the tests to leverage JUnit's `@TempDir`. Simplifies `PolarisEclipseLinkMetaStoreManagerTest` * review: rename the (now) abstract class * fix(testing): Do not let PolarisOverlappingTableTest spam `/tmp` (apache#1641) Changes the test to leverage JUnit's `@TempDir`. * Add CATALOG_MANAGE_METADATA to super privilege set of policy attachment privileges (apache#1643) * Fix quickstart doc with docker compose (apache#1610) * main: Update dependency boto3 to v1.38.22 (apache#1657) * Refactor IcebergCatalog to isolate internal state (apache#1659) Following up on apache#1694 * Restore `private` scope on internal fields in `IcebergCatalog` * Use a test-only setter instead of sub-classing to manage injecting test FileIO implementations * Refactor: Use per-request STS credentials (apache#1629) * Refactor: Use per-request STS credentials No functional changes. This is mostly to allow more storage integration flexibility in downstream build. This might also be useful for non-AWS storage. * fix and enforce more errorprone checks (apache#1663) enforces the following checks: https://errorprone.info/bugpattern/ObjectsHashCodePrimitive https://errorprone.info/bugpattern/OptionalMapToOptional https://errorprone.info/bugpattern/StringCharset https://errorprone.info/bugpattern/VariableNameSameAsType * Create a wrapper script to generate python client; regenerate the python client (apache#1347) As noted in apache#755 and elsewhere, the generated types in client/python are currently out of date. This introduces a script to regenerate them and a gradle task to run that script. I've also run the script, which necessitated several things to get tests passing: 1. There were small nonfunctional spec changes needed in order to keep the Python client working 2. The CLI and its tests required a few fixes to work with the updated Python client 3. Many of the regtests required fixes to work with the updated Python client * [Python Client] CI for Python client (Continue PR#1096) (apache#1639) Adds CI for python client. It does not include caching poetry step for now since we do not have poetry.lock (it is in .gitignore), see relevant discussion in: apache#1102 (comment), apache#1096 (comment), we can add that later * main: Update actions/setup-python action to v5 (apache#1671) * main: Update actions/checkout action to v4 (apache#1670) * main: Update python Docker tag to v3.13 (apache#1669) * main: Update dependency pytest to ~=7.4.4 (apache#1668) * main: Update dependency software.amazon.awssdk:bom to v2.31.50 (apache#1677) * main: Update dependency boto3 to v1.38.23 (apache#1667) * feat(build): make archive builds reproducible (apache#1664) See https://docs.gradle.org/current/userguide/working_with_files.html#sec:reproducible_archives * main: Update dependency io.prometheus:prometheus-metrics-exporter-servlet-jakarta to v1.3.8 (apache#1679) * NoSQL: adapt to change on oss/main * INFO: Last merged commit: 6ef8b3e --------- Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: ModEtchFill <50123102+ModEtchFill@users.noreply.github.com> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: singhpk234 <singhpk234@users.noreply.github.com> Co-authored-by: pjanuario <pjanuario@users.noreply.github.com> Co-authored-by: Honah (Jonas) J. <honahx@apache.org> Co-authored-by: MonkeyCanCode <yongzheng0809@gmail.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@dremio.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Christopher Lambert <xn137@gmx.de> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com>

Fix quickstart doc with docker compose

c125b2b

MonkeyCanCode requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike and snazy as code owners May 19, 2025 05:38

github-project-automation bot added this to Basic Kanban Board May 19, 2025

MonkeyCanCode requested review from RussellSpitzer, takidau, flyrain, ebyhr, HonahX, singhpk234 and pingtimeout as code owners May 19, 2025 05:38

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board May 19, 2025

pingtimeout previously approved these changes May 19, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 19, 2025

dimas-b reviewed May 19, 2025

View reviewed changes

getting-started/assets/.env Outdated Show resolved Hide resolved

This was referenced May 19, 2025

fix(quickstart): fixed quickstart jdbc to include client id and secrets (#1613) #1614

Closed

Quickstart getting-started/jdbc is missing Client Id and Client Secret #1613

Closed

adnanhemani suggested changes May 20, 2025

View reviewed changes

github-project-automation bot moved this from Ready to merge to PRs In Progress in Basic Kanban Board May 20, 2025

Fix quickstart doc with docker compose

6a4be53

MonkeyCanCode dismissed pingtimeout’s stale review via 6a4be53 May 20, 2025 03:34

MonkeyCanCode added 4 commits May 19, 2025 22:35

Fix quickstart doc with docker compose

f776509

Merge branch 'main' into fix_quickstart_doc

aa8dc9f

Fix quickstart doc with docker compose

1c4a0b8

Fix quickstart doc with docker compose

37b35eb

MonkeyCanCode requested a review from ajantha-bhat as a code owner May 21, 2025 05:59

MonkeyCanCode requested review from adnanhemani, dimas-b and pingtimeout May 21, 2025 06:02

adnanhemani suggested changes May 21, 2025

View reviewed changes

adnanhemani mentioned this pull request May 23, 2025

Quickstart documentation contained an error around starting Polaris using docker compose #1660

Closed

MonkeyCanCode added 3 commits May 22, 2025 22:48

Fix quickstart doc with docker compose

841ec2e

Fix quickstart doc with docker compose

f338738

Fix conflict

8fd5ff8

flyrain reviewed May 23, 2025

View reviewed changes

flyrain approved these changes May 23, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 23, 2025

flyrain merged commit 0eafcaa into apache:main May 23, 2025
6 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 23, 2025

		@@ -83,11 +89,6 @@ When using a Gradle-launched Polaris instance in this tutorial, we'll launch an
		For more information on how to configure Polaris for production usage, see the [docs]({{% relref "../configuring-polaris-for-production" %}}).

		When Polaris is run using the `./gradlew run` command, the root principal credentials are `root` and `secret` for the `CLIENT_ID` and `CLIENT_SECRET`, respectively.

Fix quickstart doc with docker compose #1610

Fix quickstart doc with docker compose #1610

Uh oh!

Conversation

MonkeyCanCode commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pingtimeout left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimas-b commented May 19, 2025

Uh oh!

adnanhemani commented May 19, 2025

Uh oh!

adnanhemani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MonkeyCanCode May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani commented May 20, 2025

Uh oh!

MonkeyCanCode commented May 21, 2025

Uh oh!

adnanhemani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MonkeyCanCode May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flyrain commented May 23, 2025

Uh oh!

Uh oh!

MonkeyCanCode commented May 19, 2025 •

edited

Loading

MonkeyCanCode May 20, 2025 •

edited

Loading

adnanhemani May 20, 2025 •

edited

Loading

MonkeyCanCode May 21, 2025 •

edited

Loading