feat(controller) Persist batch and batch dependent entities in store#119
feat(controller) Persist batch and batch dependent entities in store#119manjari25 wants to merge 3 commits into
Conversation
manjari25
requested review from
a team,
behinddwalls and
sbalabanov
as code owners
| ) | ||
| c.metricsScope.Counter("counter_errors").Inc(1) | ||
| return fmt.Errorf("failed to generate batch ID for queue=%s: %w", request.Queue, err) | ||
| return errs.NewRetryableError(fmt.Errorf("failed to generate batch ID for queue=%s: %w", request.Queue, err)) |
There was a problem hiding this comment.
plz do not default to retryables
There was a problem hiding this comment.
see other review for details
| ) | ||
|
|
||
| // Generate a globally unique batch ID. | ||
| counterOp := metrics.Begin(c.metricsScope, "counter_next") |
There was a problem hiding this comment.
counter has its one metrics, so this is probably redundant?
|
|
||
| // Persist batch to storage. | ||
| // ErrAlreadyExists should never happen since batch IDs are generated from a unique counter. | ||
| batchCreateOp := metrics.Begin(c.metricsScope, "batch_store_create") |
| batchCreateOp := metrics.Begin(c.metricsScope, "batch_store_create") | ||
| if err := c.store.GetBatchStore().Create(ctx, batch); err != nil { | ||
| batchCreateOp.Complete(err) | ||
| c.logger.Errorw("failed to create batch in storage", |
There was a problem hiding this comment.
as long as we surface error up, avoid logging as it would most likely result in double logging.
Logging should be done at where the error chain terminates, typically the highest function in the call stack
There was a problem hiding this comment.
For all QueueControllers, i guess we just log at the consumer.go and leave it as is. For API controllers, we just log at the handlers which would be implemented in go-code?
So i think we should probably never log anywhere unless we are swallowing some errors (ideally we never need to)
| "batch_id", batch.ID, | ||
| "error", err, | ||
| ) | ||
| if errors.Is(err, storage.ErrAlreadyExists) { |
There was a problem hiding this comment.
we do not expect it to ever happen if counter is correct, do not need special processing to this error, just fire it up
| // ErrAlreadyExists should never happen since batch IDs are generated from a unique counter. | ||
| batchCreateOp := metrics.Begin(c.metricsScope, "batch_store_create") | ||
| if err := c.store.GetBatchStore().Create(ctx, batch); err != nil { | ||
| batchCreateOp.Complete(err) |
There was a problem hiding this comment.
adhoc approach to scope management is fragile
what if some code path below won't return (could be another code change that makes it such). Complete would be called twice.
Prefer defer or another function wrapper to scope the measured block into.
3 participants
Summary
feat(controller) Persist batch and batch dependent entities in store
What?
Why?
Enables sequential batch ordering per queue so that downstream stages (score, speculate, merge) can process batches in the correct dependency order.
Test Plan
Issues