docs(compression-coordinator): Add a user guide for using compression-coordinator. - #2456
docs(compression-coordinator): Add a user guide for using compression-coordinator.#2456LinZhihao-723 wants to merge 14 commits into
compression-coordinator.#2456Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| Compared with `compression-scheduler`, `compression-coordinator` provides the following improvements | ||
| to the user experience, reliability, and performance: | ||
|
|
||
| * **Automatic failure recovery**: If a service fails or restarts, `compression-coordinator` | ||
| automatically resumes previously submitted jobs. Users do not need to restart them manually. | ||
| * **Improved resource utilization**: Compression jobs run in a Spider-managed execution environment, | ||
| allowing computational resources to be shared more effectively across all configured workers. | ||
| * **Configurable retries for compression failures**: `compression-coordinator` allows users to | ||
| configure a simple retry policy for failed compression tasks. Automatic retries can help recover | ||
| from transient issues, such as temporary network interruptions, without requiring user | ||
| intervention. | ||
| * TODO: Configure retry policy through <LINK> | ||
| * **Improved fairness across concurrent compression jobs**: `compression-coordinator` provides | ||
| two levels of concurrency control to improve fairness among compression jobs running concurrently: | ||
| * Coordinator-side rate limit: TODO, depends on #2435. | ||
| * Spider-side active job limit: Controls the maximum number of jobs that can make progress | ||
| concurrently while sharing compute resources in Spider. Jobs are admitted on a first-come, | ||
| first-served basis. | ||
| * **Data integrity**: Compression jobs coordinated by `compression-coordinator` publish their | ||
| results to the rest of the system through a dedicated commit stage. The commit operation is both | ||
| transactional and idempotent. | ||
| * A job-level failure does not result in partial updates. | ||
| * Internal retries do not result in duplicate updates |
There was a problem hiding this comment.
@junhaoliao Can you review this section to check if it makes sense (since it compares to the existing scheduling)?
| Compared with `compression-scheduler`, `compression-coordinator` provides the following improvements | ||
| to the user experience, reliability, and performance: | ||
|
|
||
| * **Automatic failure recovery**: If a service fails or restarts, `compression-coordinator` |
There was a problem hiding this comment.
Before we edit---
-
What service?
-
fails or restarts
Does that imply a service could fail but not restart and
compression-coordinatorwould still resume the job?
There was a problem hiding this comment.
- For 1: My bad. The service should only refer to
compression-coordinator. - For 2: If the service is
compression-coordinatorin specific, this sentence only makes sense by assumingcompression-coordinatorrestarts. - Shall we restructure this sentence to indicate that Spider's service failure is automatically handled as well? But I'm not sure if we need to assume a Spider service fails infinitely without getting recovered.
There was a problem hiding this comment.
Shall we restructure this sentence to indicate that Spider's service failure is automatically handled as well?
Perhaps we can add a few sub-bullets under "Automatic failure recovery:". One about compression-coordinator and one about Spider being fault-tolerant itself?
But I'm not sure if we need to assume a Spider service fails infinitely without getting recovered.
I think that's outside the scope of reasonable failure recovery, right? Like failure recovery also has its limits.
There was a problem hiding this comment.
- Sure. We can explain
compression-coordinatorand Spider separately. - Fair. I have the same intuition but just to confirm.
| * **Data integrity**: Compression jobs coordinated by `compression-coordinator` publish their | ||
| results to the rest of the system through a dedicated commit stage. The commit operation is both | ||
| transactional and idempotent. | ||
| * A job-level failure does not result in partial updates. |
There was a problem hiding this comment.
Although, the indexer table still gets updated, right? Maybe we want to warn about that so that no one gets surprised?
There was a problem hiding this comment.
That is correct. This is mainly talking about archive metadata and omitting indexer metadata intentionally. Indexer metadata consistency is already misleading in some sense:
- In the current implementation, a compression task that fails to update the indexer table is still considered successful.
- When an archive is removed by retention-based GC, the indexer table is not updated accordingly.
This means the indexer table can:
- Be incomplete about all possible schemas of existing archives.
- Contain schemas that don't exist in any of the archives.
compression-coordinator doesn't solve any of these. And to document this behavior, it probably needs a separate section... not sure if we want to cover it here.
Back to this bullet point, how about we be explicit that this guarantee is only for archive metadata?
There was a problem hiding this comment.
Sure, we can be explicit, but I think it's still worth adding a short warning to say this doesn't apply to data in the <dataset>_column_metadata tables. Actually, maybe we should open a GitHub issue and then just link to that.
There was a problem hiding this comment.
@coderabbitai Create an issue to track this.
- Document the inconsistency introduced by the discussed behavior.
- Document the proposed solution for the all-or-nothing update: make
indexerpass its results to the compression jobs' commit stage. - Document we should come up with a solution to keep track of the removed column metadata in archive GC.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gh issue list \
--repo y-scope/clp \
--state open \
--limit 100 \
--search '"column metadata" indexer OR "archive GC" OR "commit stage"' \
--json number,title,url,body \
--jq '.[] | {number, title, url, body}'Length of output: 391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
body=$(cat <<'EOF'
## Summary
Track consistency gaps in indexer column metadata. The archive metadata commit is transactional and idempotent, but the `<dataset>_column_metadata` tables do not have the same consistency guarantee.
## Current inconsistency
- A compression task can succeed when its update to the indexer table fails.
- Retention-based archive GC removes archives without removing their related column metadata from the indexer table.
- Therefore, `<dataset>_column_metadata` can omit schemas for existing archives and contain schemas for archives that no longer exist.
## Required work
1. Document the current consistency behaviour and its limits for `<dataset>_column_metadata`.
2. Design and implement an all-or-nothing update path. The `indexer` should pass its results to the compression job commit stage so that archive metadata and indexer metadata commit together.
3. Design and implement a method for archive GC to identify and remove column metadata that belongs to removed archives.
## Affected areas
- Compression jobs and their commit stage.
- `indexer` result handling and indexer-table updates.
- Retention-based archive GC.
- User documentation for compression and indexer metadata consistency.
## Acceptance criteria
- Documentation states that the archive metadata guarantee does not currently apply to `<dataset>_column_metadata`.
- The proposed implementation defines failure and retry behaviour for combined archive and indexer metadata updates.
- Compression jobs do not report success after an indexer metadata update failure when all-or-nothing semantics are enabled.
- Archive GC keeps column metadata consistent with removed archives.
- Tests cover compression failure, indexer update failure, retry, and archive GC cases.
## Context
Requested by `@LinZhihao-723` during review of [PR `#2456`](https://github.com/y-scope/clp/pull/2456).
Discussion: https://github.com/y-scope/clp/pull/2456#discussion_r3752018113
EOF
)
gh issue create \
--repo y-scope/clp \
--title 'Keep indexer column metadata consistent with archive lifecycle' \
--body "$body" \
--assignee LinZhihao-723Length of output: 2343
@LinZhihao-723, created and assigned #2480: Keep indexer column metadata consistent with archive lifecycle.
You are interacting with an AI system.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
| * **Automatic failure recovery**: | ||
| * `compression-coordinator` automatically resumes previously submitted jobs (users don't need to | ||
| restart them manually). | ||
| * Spider's fault-tolerance model allows it to recover transparently from internal failures without | ||
| requiring user intervention. |
There was a problem hiding this comment.
Split into two bulllet points for compression-coordinator and Spider.
| * **Easier failure handling via a configurable retry policy**: `compression-coordinator` allows | ||
| users to configure the maximum number of retries for each compression task (retries are | ||
| unconditional). This can help tasks recover automatically from transient failures, such as | ||
| temporary network connectivity issues. | ||
| * TODO: Link to #2457. |
There was a problem hiding this comment.
- Simplified the writing a bit.
- Mentioned that the retry is unconditional.
| * **Improved resource utilization**: `compression-scheduler` processes tasks in batches, where each | ||
| batch must wait for its slowest task to finish. `compression-coordinator` instead schedules | ||
| individual tasks through Spider, allowing resources to be reassigned as soon as individual tasks | ||
| finish. |
There was a problem hiding this comment.
Be more specific about batch-granularity vs. task-granularity.
| * **Bounded job concurrency**: `compression-coordinator` limits the number of compression jobs that | ||
| can be submitted to Spider concurrently. In contrast, `compression-scheduler` does not bound the | ||
| number of concurrent jobs, which can cause scheduling overhead to grow significantly at high | ||
| levels of job concurrency. |
There was a problem hiding this comment.
Split this from "fairness".
| * `compression-scheduler` achieves similar job-level fairness only when | ||
| `max_concurrent_tasks_per_job` is set to 1, effectively scheduling one task per job in | ||
| round-robin order. However, this round-robin scheduling spans an unbounded number of concurrent | ||
| jobs and can incur significant overhead at high concurrency. Spider provides similar fairness | ||
| with much lower scheduling overhead through a more efficient architecture and a bounded set of | ||
| active jobs. |
There was a problem hiding this comment.
iiuc, this is the most precise description of compression-scheduler's behavior: only if max_concurrent_tasks_per_job is 1, it can do a task-level round-robin. But that means jobs are reported at the task level in the metadata DB, whose overhead should be as bad as Spider's C++ implementation.
I'm not sure if we need this sub-bullet point.
| * **Improved all-or-nothing semantics**: Compression jobs coordinated by `compression-coordinator` | ||
| publish all archive metadata to the `clp_<dataset>_archives` table in a single dedicated commit | ||
| operation. The commit operation is both transactional and idempotent, so: | ||
| * a job-level failure doesn't result in partial updates. | ||
| * internal task retries don't result in duplicate updates. |
There was a problem hiding this comment.
Be specific that the atomic updates is on clp_<dataset>_archives table.
| The **all-or-nothing semantics** do not apply to the column metadata table | ||
| (`<dataset>_column_metadata`), because it is updated during compression job execution. If a | ||
| compression job fails, this table may contain partial updates. This known limitation is tracked in | ||
| [this GitHub issue][column-metadata-issue]. |
There was a problem hiding this comment.
A small section warning the column metadata updates is not "all-or-nothing".
There was a problem hiding this comment.
Might need a few more touch-ups after this round of review.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Description
This PR depends on #2455.
Checklist
breaking change.
Validation performed