-
Notifications
You must be signed in to change notification settings - Fork 88
docs(presto-clp): Add instructions for connecting CLP's UI to Presto; Simplify some set up instructions. #1310
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
Merged
Merged
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
2e336cb
latest
828f55c
latest
d38cbe7
latest
83c9016
Apply suggestions from code review
davemarco 2156ce1
latest
da3942e
latest
3bab531
latest
6b4c82a
Apply suggestions from code review
davemarco 832ffb2
latest
f3043fe
latest
d8ad955
Apply suggestions from code review
davemarco f9ec9a6
latest
d495f97
latest
43a39cc
Update docs/src/user-docs/guides-using-presto.md
davemarco e09ef68
latest
51ded2a
Merge branch 'pdocs' of https://github.com/davemarco/clp into pdocs
b61b55d
Update docs/src/user-docs/guides-using-presto.md
davemarco 9d82ad9
Merge branch 'main' into pdocs
davemarco e394803
Merge branch 'main' into pdocs
davemarco File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -33,27 +33,60 @@ Using Presto with CLP requires: | |||||
|
|
||||||
| 1. Follow the [quick-start](quick-start/index.md) guide to download and extract the CLP package, | ||||||
| but don't start the package just yet. | ||||||
| 2. Before starting the package, update the package's config as follows: | ||||||
| 2. Before starting the package, update the package's config file (`etc/clp-config.yml`) as follows: | ||||||
|
|
||||||
| * Open `etc/clp-config.yml` located within the package. | ||||||
| * Uncomment the `database` section. | ||||||
| * Change `database.host` value to a non-localhost hostname/IP. | ||||||
| * After the change, the `database` section should look something like this: | ||||||
| * Set the `package.query_engine` key to `"presto"`. | ||||||
|
|
||||||
| ```yaml | ||||||
| database: | ||||||
| type: "mariadb" # "mariadb" or "mysql" | ||||||
| host: "<new-IP-address>" | ||||||
| port: 3306 | ||||||
| name: "clp-db" | ||||||
| package: | ||||||
| storage_engine: "clp-s" | ||||||
| query_engine: "presto" | ||||||
| ``` | ||||||
|
|
||||||
| :::{note} | ||||||
| This change is necessary since the Presto containers run on a Docker network, whereas CLP's | ||||||
| database runs on the host network. So `localhost` refers to two different entities in those | ||||||
| networks. This limitation will be addressed in the future when we unify Presto and CLP's | ||||||
| deployment infrastructure. | ||||||
| ::: | ||||||
| * Set the `database.host` key to a non-localhost hostname/IP. | ||||||
|
|
||||||
| ```yaml | ||||||
| database: | ||||||
| # type: "mariadb" | ||||||
| host: "<non-local-ip-address>" | ||||||
| # port: 3306 | ||||||
| # name: "clp-db" | ||||||
| ``` | ||||||
|
|
||||||
| :::{note} | ||||||
| This change is necessary because the Presto containers run on a Docker network, and CLP's | ||||||
| database runs on the host network. `localhost` will refer to a different entity in each of | ||||||
| those contexts. This limitation will be addressed in the future when we unify Presto and CLP's | ||||||
| deployment infrastructure. | ||||||
| ::: | ||||||
|
|
||||||
| * Set the `results_cache.retention_period` key to `null` since the CLP + Presto integration | ||||||
| doesn't yet support for garbage collection. | ||||||
|
|
||||||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| ```yaml | ||||||
| results_cache: | ||||||
| # host: "localhost" | ||||||
| # port: 27017 | ||||||
| # db_name: "clp-query-results" | ||||||
| # stream_collection_name: "stream-files" | ||||||
| # | ||||||
| # # Retention for search results, in minutes. Set to null to disable automatic deletion. | ||||||
| retention_period: null | ||||||
|
||||||
| retention_period: null | |
| retention_period: null |
Contributor
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i changed the whole indenting for this block in another commit
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
quinntaylormitchell marked this conversation as resolved.
Show resolved
Hide resolved
quinntaylormitchell marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i figure this docs PR should provide better context, although the code for the Presto Docker Compose project was originally added in #1132.)
The mitigation documented above is practical but may be inconvenient for users operating in diverse network environments.
In the long term, we plan to integrate the Presto services into the CLP Package Docker Compose project introduced in #1178. Running
du -h tools/deployment/presto-clpshows the directory is only 132 KB, indicating that the integration would not significantly increase the package size while substantially improving user convenience.@coderabbitai please file an issue to track this long-term integration plan.
Since implementing and reviewing this integration may take time, a short-term solution is needed before the next release. After #1178, the
webuiservice may also encounter connectivity issues with the Presto connector, as host network mode was removed in that update.A likely mitigation is to allow the Presto project to attach to an external Docker network defined in the CLP Package Compose project.
@coderabbitai please create a separate issue to track this short-term fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 229
@junhaoliao, I've created both issues as requested:
Long-term integration: #1497 - Integrate Presto services into CLP Package Docker Compose project
Short-term fix: #1498 - Allow Presto project to attach to external Docker network for CLP Package Compose
Both issues have been assigned to you and include references to PR #1310 and the relevant context.
🧠 Learnings used