feat(controller): hooking up merge controller by kevinlnew · Pull Request #105 · uber/submitqueue

kevinlnew · 2026-02-27T20:01:35Z

Summary

feat(controller): hooking up merge controller

What?
Hooked up the merge controller with the land provider.
Also wired up the example server with the merge controller along with default github land provider.

Idempotency checks:

Batch state
- if batch is already terminal, this means we can skip landing and store updates and publish the message.
- otherwise, try to land.
Landing:
- Implementation can pass back ErrIsAlreadyLanded, which is a happy path signal to set the batch to succeeded.
Errors
- All the returned errors except bad payload and ErrLandRejected are retryable.

Test Plan

Unit tests, CI

Issues

Stack

sbalabanov · 2026-02-28T00:05:26Z

+	// BatchStateLandFailed is the state of a batch that has failed to land.
+	BatchStateLandFailed BatchState = "land_failed"
+	// BatchStateLandSucceeded is the state of a batch that has successfully landed.
+	BatchStateLandSucceeded BatchState = "land_succeeded"


How does it differ from "succeeded" ?

Removed land_failed and land_succeeded, they weren't necessary. Just artifacts from me trying to understand state transitions.
The merge controller now just updates the batch to the terminal states (succeeded and failed) for the "concluder" controller to pick up later.

sbalabanov · 2026-02-28T00:14:37Z

+		)
+		c.metricsScope.Counter("idempotent_skip").Inc(1)
+	case entity.BatchStateLanding:
+		newState, err := c.land(ctx, batch)


It should also process the land error "already merged" for another idempotency check.
If it is for the future, add TODO comment.

Added the IsAlreadyLanded error to the interface in a previous PR in the stack and hooked it up to the land function

…ller

…te logical flow for idempotency

sbalabanov · 2026-03-04T02:04:18Z

 	c.metricsScope.Counter("received").Inc(1)

 	msg := delivery.Message()
+	batchID := string(msg.Payload)


We need to define a serialization format for this. Using direct string-to-byte conversion is Golang-specific.
Probably a separate serializable structure.

sbalabanov · 2026-03-04T02:07:46Z

+	// previous attempt; skip straight to publish.
+	batch, err := c.storage.GetBatchStore().Get(ctx, batchID)
+	if err != nil {
+		c.logger.Errorw("failed to fetch batch",


as long as we return error to upstream, we do not have to log. Otherwise most likely we'll log twice.
Logging should happen at error chain termination (top-most) point.

sbalabanov · 2026-03-04T02:08:33Z

 		)
-		c.metricsScope.Counter("publish_errors").Inc(1)
-		return errs.NewRetryableError(fmt.Errorf("failed to publish to merge-signal: %w", err))
+		c.metricsScope.Counter("batch_fetch_errors").Inc(1)


plz rebase and integrate with new metrics infrastructure that processes errors automagically

sbalabanov · 2026-03-04T02:13:31Z

+				"error", err,
+			)
+			c.metricsScope.Counter("batch_update_errors").Inc(1)
+			return errs.NewRetryableError(fmt.Errorf("failed to update batch state: %w", err))


it should only be retryable if the error is expected type, like version mismatch, but we seem to retry on all of the errors? If the database is operational but "table does not exist" is the error, this is not retryable.

sbalabanov · 2026-03-04T02:15:04Z

+
+		batch.State = newState
+	} else {
+		c.logger.Infow("batch already in terminal state, skipping to publish",


may be a warning level, as this should only happen on infra errors

sbalabanov · 2026-03-04T02:20:28Z

+	err := c.landProvider.Land(ctx, batch.Queue, entries)
+
+	switch {
+	case err == nil, landprovider.IsAlreadyLanded(err):


may be at least log landprovider.IsAlreadyLanded, but better tally too?
this also should not happen for a normal workflow, so good to have some observability
also, comment here scenarios which could cause this

sbalabanov · 2026-03-04T02:22:10Z

+			"error", err,
+		)
+		c.metricsScope.Counter("land_rejected").Inc(1)
+		return entity.BatchStateFailed, nil


we'll need to propagate the error information about why land was rejected. Can be done in another diff, but then needs a TODO comment.

kevinlnew mentioned this pull request Feb 27, 2026

feat(landprovider): Adding land provider extension interface #104

Closed

kevinlnew force-pushed the kevin.new/landprovider branch from 2cffebf to e8b31bc Compare February 27, 2026 21:46

kevinlnew force-pushed the kevin.new/mergecontroller branch from 0815e08 to ec6b0b5 Compare February 27, 2026 21:46

kevinlnew force-pushed the kevin.new/landprovider branch from e8b31bc to bfbfc86 Compare February 27, 2026 21:57

kevinlnew force-pushed the kevin.new/mergecontroller branch from ec6b0b5 to 6c6c3e3 Compare February 27, 2026 22:11

kevinlnew changed the title ~~feat(controller): hooking up merge controller~~ [WIP] feat(controller): hooking up merge controller Feb 27, 2026

sbalabanov requested changes Feb 28, 2026

View reviewed changes

kevinlnew force-pushed the kevin.new/landprovider branch from bfbfc86 to c78ac99 Compare March 2, 2026 18:25

kevinlnew force-pushed the kevin.new/mergecontroller branch from 6c6c3e3 to 40bfe00 Compare March 2, 2026 18:25

kevinlnew changed the base branch from kevin.new/landprovider to kevin.new/github-landprovider March 2, 2026 18:25

kevinlnew requested a review from sbalabanov March 2, 2026 18:25

kevinlnew mentioned this pull request Mar 2, 2026

feat(landprovider): basic github implementation #109

Closed

kevinlnew force-pushed the kevin.new/github-landprovider branch from 28f56d9 to d9f72cd Compare March 2, 2026 18:31

kevinlnew force-pushed the kevin.new/mergecontroller branch 3 times, most recently from b6bd849 to bab9708 Compare March 2, 2026 20:41

kevinlnew force-pushed the kevin.new/github-landprovider branch from d511180 to 32190ca Compare March 2, 2026 20:41

kevinlnew added 4 commits March 2, 2026 20:54

feat(controller): hooking up merge controller

3090b30

WIP change message to batch ids

fb603fc

added landProvider factory and wired up with orchestrator/mergecontro…

10669da

…ller

fixes: remove unncessary batch states, update errs to retryable, upda…

93bcc1f

…te logical flow for idempotency

kevinlnew force-pushed the kevin.new/mergecontroller branch from bab9708 to 93bcc1f Compare March 2, 2026 20:54

add request ids to logging in happy path

f9a2c00

kevinlnew marked this pull request as ready for review March 2, 2026 22:29

kevinlnew requested review from a team and behinddwalls as code owners March 2, 2026 22:29

kevinlnew changed the title ~~[WIP] feat(controller): hooking up merge controller~~ feat(controller): hooking up merge controller Mar 3, 2026

sbalabanov requested changes Mar 4, 2026

View reviewed changes

behinddwalls closed this May 28, 2026

Conversation

kevinlnew commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Issues

Stack

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

kevinlnew commented Feb 27, 2026 •

edited

Loading