feat(controller) Score controller scores batches using heuristic scorer by manjari25 · Pull Request #121 · uber/submitqueue

manjari25 · 2026-03-05T01:32:22Z

Summary

feat(controller) Score controller scores batches using heuristic scorer

What?

Add concept of Score to batches
- Add Score field to batch entity
- Batch store schema change
- Update existing Batch store methods to account for new schema
- Add new UpdateScoreAndState method
Add heuristic scorer to orchestrator service
Use this scorer in the batch scorer controller to score a batch
Once scored, update the batch store with the score and state
Use newly added core/metrics pkg for metric emission

Why?

Having a score associated with a batch is needed so that in the speculation stage; speculation path scores can be computed based on this number.
A speculation path score will be computed based on the success/failure score of all batches in the path.

Test Plan

Issues

sbalabanov · 2026-03-05T23:08:46Z

+func (b Batch) WithScoreAndState(score float32, state BatchState) Batch {
+	b.Score = score
+	b.State = state
+	b.Version++


why need to increment the version? It should only be incremented after a successful atomic persistence (i.e. a write to the database).

Now I see where it is used (after the update), the comment could reflect that.

Added a comment.

i am wondering if this should be in entity at all?

to me it seems like we probably need this to be part of contract at storage layer itself which accepts, entity, oldVersion, newVresion... I am not fan of implicit things as they hide away and lead to confusion/issues down the line..

Thoughts?

sbalabanov · 2026-03-05T23:09:50Z

+func newScorer(scope tally.Scope) scorer.Scorer {
+	return heuristicscorer.New(
+		[]heuristicscorer.Bucket{
+			{Min: 0, Max: 100, Score: 1.0},


add "TODO" for the real one?

sbalabanov · 2026-03-05T23:10:52Z

    queue VARCHAR(255) NOT NULL,
    contains JSON NOT NULL,
    dependencies JSON NOT NULL,
+    score FLOAT NOT NULL DEFAULT 0,


avoid db defaults, leave it to application

sbalabanov · 2026-03-05T23:20:45Z

+			"error", err,
+		)
+		metrics.NamedCounter(c.metricsScope, "process", "storage_errors", 1)
+		return errs.NewRetryableError(fmt.Errorf("failed to get request %s: %w", batch.Contains[0], err))


Only explicitly known errors should be converted to retryable.
For example, if database connection fails, it is retryable.
If database schema mismatch, it is not retryable.
Being not retryable is a default to avoid retry storms.
For now we can just bubble up the error unchanged with a TODO to make a classification later.

This isn't only schema mismatch though right? Get from the store could fail for any other reason as well. Do we not want to retry in this case?

Our default should be to not retry. Retryable errors need to be explicitly opted in as such.
For now we can put TODO here, return generic err and classify later, I envision there may be a general helper framework needed to transform i.e. mysql errors to retryable or not.

sbalabanov · 2026-03-05T23:22:46Z

+	// Update batch store with score and transition state to speculating
+	if err := c.store.GetBatchStore().UpdateScoreAndState(ctx, batch.ID, batch.Version, batchScore, entity.BatchStateSpeculating); err != nil {
+		if errors.Is(err, storage.ErrVersionMismatch) {
+			c.logger.Errorw("version mismatch updating batch score",


If it is expected by some workflow path, should be warning at most

I don't think we expect this by any workflow path...

In this case just bubble the error up without processing?

sbalabanov · 2026-03-05T23:24:27Z

+
+	// Update batch store with score and transition state to speculating
+	if err := c.store.GetBatchStore().UpdateScoreAndState(ctx, batch.ID, batch.Version, batchScore, entity.BatchStateSpeculating); err != nil {
+		if errors.Is(err, storage.ErrVersionMismatch) {


nit: we probably should create wrappers for it in storage.go

Will work on this in a different PR (so we can get this one out before the grand mock refactor).

sbalabanov · 2026-03-05T23:27:00Z


 	// Publish to speculate topic
-	if err := c.publish(ctx, consumer.TopicKeySpeculate, batch); err != nil {
+	if err := c.publish(ctx, consumer.TopicKeySpeculate, scored); err != nil {


I think we publish batch_id, not the whole object

We do - that is abstracted in func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, batch entity.Batch) error function below

sbalabanov · 2026-03-06T21:38:05Z

+			metrics.NamedCounter(c.metricsScope, "process", "request_not_found_errors", 1)
+			return fmt.Errorf("request %s not found: %w", batch.Contains[0], err)
+		}
+		c.logger.Errorw("failed to get request",


no need to log if bubble up

sbalabanov · 2026-03-06T21:38:19Z

+	request, err := c.store.GetRequestStore().Get(ctx, batch.Contains[0])
+	if err != nil {
+		if storage.IsNotFound(err) {
+			c.logger.Errorw("request not found",


no need to log if bubble up

sbalabanov · 2026-03-06T21:40:20Z

+	// Look up the request to get its Change
+	request, err := c.store.GetRequestStore().Get(ctx, batch.Contains[0])
+	if err != nil {
+		if storage.IsNotFound(err) {


do we need to process it separately? why not to just to bubble up? This is not an expected state, even when states are (eventually) inconsistent this should not be true.

sbalabanov · 2026-03-06T21:41:45Z

+	// Score the change
+	score, err := c.scorer.Score(ctx, request.Change)
+	if err != nil {
+		c.logger.Errorw("failed to score change",


ditto
plz check other usages

sbalabanov · 2026-03-06T21:43:02Z

+			"error", err,
+		)
+		metrics.NamedCounter(c.metricsScope, "process", "batch_store_errors", 1)
+		return errs.NewRetryableError(fmt.Errorf("failed to update batch %s score: %w", batch.ID, err))


ditto, plz check other usages

manjari25 added 2 commits March 5, 2026 01:24

feat(controller) Score controller scores batches using heuristic scorer

9d08ea6

use core/metrics

f5a9fcb

manjari25 marked this pull request as ready for review March 5, 2026 01:38

manjari25 requested review from a team, behinddwalls and sbalabanov as code owners March 5, 2026 01:38

sbalabanov requested changes Mar 5, 2026

View reviewed changes

address comments

33fcaab

sbalabanov requested changes Mar 6, 2026

View reviewed changes

behinddwalls closed this May 28, 2026

Conversation

manjari25 commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What?

Why?

Test Plan

Issues

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!

behinddwalls Mar 7, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

sbalabanov Mar 6, 2026 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

manjari25 commented Mar 5, 2026 •

edited

Loading

behinddwalls Mar 7, 2026 •

edited

Loading

sbalabanov Mar 6, 2026 •

edited

Loading