Skip to content

feat(compactor HS): add job queue and deletion job builder - #17843

Merged
sandeepsukhani merged 11 commits into
mainfrom
compactor-hs-add-job-queue-and-builder
May 29, 2025
Merged

feat(compactor HS): add job queue and deletion job builder#17843
sandeepsukhani merged 11 commits into
mainfrom
compactor-hs-add-job-queue-and-builder

Conversation

@sandeepsukhani

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
As a follow-up work on making Compactor horizontally scalable for processing of delete requests, this PR adds the following features:

  1. Adds a job queue which would be exposed over grpc to pull the jobs and get back job execution responses from the worker. It retries the jobs after a 15min timeout or does up to 3 retries if we get error response in the job execution status.
  2. Adds a job builder which builds jobs with up to 1K chunks in each job. It relies on deletion manifests built by the deletion manifest builder added in PR feat: Compactor deletion manifest builder #17474. A manifest is treated as complete only when a manifest.json file exists in the manifest directory. If we get an error in job execution status update, we halt building jobs from the current manifest and retry them in the next attempt.

Special notes for your reviewer:

  1. None of the code is wired to the compactor, so it should hopefully not break anything.
  2. I have converted the chunk ID in the retention.Chunk from byte slice to a string to simplify the new code. The byte slice was added initially to optimise memory usage with boltdb index. However, since we recommend tsdb and byte slice optimisation doesn't help much with it, I have changed the type.

Checklist

  • Tests updated
@sandeepsukhani
sandeepsukhani requested a review from a team as a code owner May 28, 2025 13:39
Comment thread pkg/compactor/jobqueue/queue.proto Outdated
// Dequeue retrieves the next job from the queue
rpc Dequeue(DequeueRequest) returns (DequeueResponse) {}
// ReportJobResponse reports the result of executing a job
rpc ReportJobResponse(ReportJobResultResponse) returns (ReportJobResultRequest) {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This feels backward? Shouldn't it take a request and return a response?

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.

good catch. I don't know how I made that mistake 🤦

Comment thread pkg/compactor/jobqueue/queue.go Outdated
// Queue implements the job queue service
type Queue struct {
queue chan *Job
closed bool

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This feels like it might lead to a race condition? I couldn't create one, but this boolean is accessed multiple places without any synchronization.

Comment thread pkg/compactor/jobqueue/queue.go Outdated
stop: make(chan struct{}),
checkTimedOutJobsInterval: checkTimedOutJobsInterval,
processingJobs: make(map[string]*processingJob),
jobTimeout: 15 * time.Minute,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to make the queue size, the timeout, or the retry max configurable?

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 didn't want to make the queue a buffered channel to build the jobs as they are picked up for processes, but I missed making the change. When any jobs fail, we want to ensure we have as few jobs in flight as possible because we stop processing manifests and abandon all the running jobs for that manifest.

We can certainly make the other two configurable, but since the code is not wired in, I have just left a TODO for now.

@sandeepsukhani
sandeepsukhani requested a review from paul1r May 29, 2025 08:09
@sandeepsukhani
sandeepsukhani merged commit 267f4dc into main May 29, 2025
@sandeepsukhani
sandeepsukhani deleted the compactor-hs-add-job-queue-and-builder branch May 29, 2025 12:08
felix0102 pushed a commit to felix0102/loki that referenced this pull request Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants