Skip to content

docs(clp-config): Add commented out default values to nullable mcp_server and presto in clp-config.yaml; Clarify configuration steps in the manuals. #1635

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

Open
junhaoliao wants to merge 6 commits into
base: main
Choose a base branch
from
Open

docs(clp-config): Add commented out default values to nullable mcp_server and presto in clp-config.yaml; Clarify configuration steps in the manuals. #1635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
99079ac
docs(clp-config): Add commented out default values to nullable `mcp_s…
junhaoliao Nov 19, 2025
f9c97e1
Merge branch 'main' into nullable-config-docs
junhaoliao Nov 20, 2025
a01fc32
Merge branch 'main' into nullable-config-docs
junhaoliao Feb 17, 2026
d9eaff0
docs(config): Update Presto client configuration placeholders for cla…
junhaoliao Feb 17, 2026
f6ea242
Merge branch 'main' into nullable-config-docs
junhaoliao Feb 17, 2026
c85afa3
add mcp values to json.yaml as well
junhaoliao Feb 17, 2026
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
4 changes: 4 additions & 0 deletions components/package-template/src/etc/clp-config.template.json.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@
# rate_limit: 1000
#
#mcp_server: null
## host: "localhost"
## port: 8000
#
## Where archives should be output to
#archive_output:
Expand Down Expand Up @@ -156,6 +158,8 @@
#
## Presto client config
#presto: null
## host: <presto-coordinator-ip> # default: "localhost"
## port: <presto-coordinator-port> # default: 8889
Comment on lines 160 to +162
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Inconsistent placeholder style compared to mcp_server section.

Lines 108–109 for mcp_server show actual default values ("localhost", 8000), while here angle-bracket placeholders are used with defaults relegated to trailing comments. Consider using the same pattern for both sections — i.e., showing the actual defaults directly and noting they can be replaced:

Suggested diff 
 `#presto`: null
-##  host: <presto-coordinator-ip>  # default: "localhost"
-##  port: <presto-coordinator-port>  # default: 8889
+##  host: "localhost"
+##  port: 8889

This would also avoid leaving invalid YAML tokens (<...>) that would cause a parse error if a user uncomments the block without replacing them.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#presto: null
## host: <presto-coordinator-ip> # default: "localhost"
## port: <presto-coordinator-port> # default: 8889
`#presto`: null
## host: "localhost"
## port: 8889
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@components/package-template/src/etc/clp-config.template.json.yaml` around
lines 160 - 162, The presto section uses angle-bracket placeholders that are
inconsistent with the mcp_server section and can create invalid YAML; change the
presto block (keys: presto.host and presto.port) to show the actual default
values (e.g., "localhost" and 8889) as the values (not angle-bracket tokens) and
keep a short inline comment indicating they can be replaced, mirroring how
mcp_server is represented so uncommenting the block won't introduce invalid
tokens.

#
## Location where other data (besides archives) are stored. It will be created if
## it doesn't exist.
Expand Down
4 changes: 4 additions & 0 deletions components/package-template/src/etc/clp-config.template.text.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ log_ingestor: null
# rate_limit: 1000
#
#mcp_server: null
## host: "localhost"
## port: 8000
#
## Where archives should be output to
#archive_output:
Expand Down Expand Up @@ -139,6 +141,8 @@ log_ingestor: null
#
## Presto client config
#presto: null
## host: <presto-coordinator-ip> # default: "localhost"
## port: <presto-coordinator-port> # default: 8889
#
## Location where other data (besides archives) are stored. It will be created if
## it doesn't exist.
Expand Down
17 changes: 5 additions & 12 deletions docs/src/user-docs/guides-mcp-server/index.md
View file
Open in desktop
Copy link
Member Author

@junhaoliao junhaoliao Nov 26, 2025

Choose a reason for hiding this comment

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

@rishikeshdevsot may i get your approval for the modifications in this docs if they look good to you?

Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,9 @@ This guide assumes:

## Starting the MCP Server

1. Configure `clp-json` to run the MCP server by locating the `mcp_server` section in
`etc/clp-config.yaml`

```yaml
#mcp_server: null
```

then uncommenting it and specifying a `host` and `port`:
1. Configure `clp-json` to run the MCP server by uncommenting the `mcp_server` section in
`etc/clp-config.yaml` and specifying a `host` and `port`. Replace the default values if
necessary.

```yaml
mcp_server:
Expand All @@ -37,11 +32,9 @@ This guide assumes:
# other settings
```

Replace the default `host` and `port` shown above if necessary.

:::{note}
If you set `mcp_server: null` or leave the `mcp_server` section commented, the MCP server won't
be started with the rest of `clp-json`.
Setting `mcp_server: null` or leaving the section commented will prevent the MCP server from
starting with `clp-json`.
:::

2. Start `clp-json` and compress the logs you want to query by following the [clp-json
Expand Down
9 changes: 4 additions & 5 deletions docs/src/user-docs/guides-using-presto.md
View file
Open in desktop
Copy link
Member Author

@junhaoliao junhaoliao Nov 26, 2025

Choose a reason for hiding this comment

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

@davemarco may i get your approval for the modifications in this docs if they look good to you?

Copy link
Contributor

@davemarco davemarco Nov 26, 2025

Choose a reason for hiding this comment

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

I think haiqi originally did not want to put the "default" port since there is no error when starting the package if the port is incorrect. Like he wanted to force the user to input their specific port

Copy link
Member Author

@junhaoliao junhaoliao Nov 28, 2025

Choose a reason for hiding this comment

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

hmm.. even if we ask users to provide a port, if we don't do any checks (i guess we really can't), there is no guarantee that the port is valid?

fwiw, i believe providing a default helps avoid collision with ports of other services in the CLP package / other well-known ports

Copy link
Member Author

@junhaoliao junhaoliao Feb 17, 2026

Choose a reason for hiding this comment

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

@davemarco / @kirkrodrigues could you check if the latest proposal would address those concerns of users using non-standard ports

Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,13 @@ Using Presto with CLP requires:
retention_period: null
```

* Update the `presto` key with the host and port of the Presto cluster. If you follow the
[Setting up Presto](#setting-up-presto) section, the host is `localhost` and the port is
`8889`.
* Update the `presto` key with the `host` and `port` of the Presto cluster. Replace the default
Copy link
Member Author

@junhaoliao junhaoliao Feb 17, 2026

Choose a reason for hiding this comment

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

Suggested change
* Update the `presto` key with the `host` and `port` of the Presto cluster. Replace the default
* Update the `presto` key with the `host` and `port` of the Presto coordinator. Replace the default

values if necessary.

```yaml
presto:
host: "<ip-address>"
port: <port>
host: "localhost"
port: 8889
Copy link
Member Author

@junhaoliao junhaoliao Feb 17, 2026

Choose a reason for hiding this comment

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

Suggested change
port: 8889
host: <presto-coordinator-ip> # default: "localhost"
port: <presto-coordinator-port> # default: 8889

```

:::{note}
Expand Down
Loading