[3/N] [List API] feat(summary): project request logs from admission context#303
[3/N] [List API] feat(summary): project request logs from admission context#303albertywu wants to merge 1 commit into
Conversation
albertywu
force-pushed
the
wua/list-api-request-log
branch
from
e8b3462 to
e60d532
Compare
albertywu
force-pushed
the
wua/list-api-summary-store
branch
2 times, most recently
from
552ce82 to
419a487
Compare
albertywu
force-pushed
the
wua/list-api-request-log
branch
2 times, most recently
from
4255521 to
bf69e42
Compare
albertywu
force-pushed
the
wua/list-api-summary-store
branch
from
419a487 to
4980cb0
Compare
albertywu
requested review from
a team,
behinddwalls and
sbalabanov
as code owners
albertywu
force-pushed
the
wua/list-api-request-log
branch
from
bf69e42 to
5fb143c
Compare
albertywu
force-pushed
the
wua/list-api-summary-store
branch
from
4980cb0 to
788ef8d
Compare
| // event — invalidates the read, in which case we re-read and re-merge. Merges are monotonic (the | ||
| // winner only advances by request version or timestamp), so the loop converges; re-applying a log | ||
| // that has already been merged is a no-op. | ||
| func (s *requestSummaryStore) UpsertFromLog(ctx context.Context, log entity.RequestLog) (retErr error) { |
There was a problem hiding this comment.
ideally this is purely a write to DB, like any other store with CAS, any logic to summarize should live in the request logs consumer controller, we can event thing of adding more queues if needed post persisting requestLog to summarize and do any other things as needed. But we can start with requestlog controller itself
| StartedAtMs int64 | ||
| RequestID string |
There was a problem hiding this comment.
do we need both or just the last RequestID we returned as cursor?
| StartedAtMs int64 | ||
| UpdatedAtMs int64 | ||
| CompletedAtMs int64 | ||
| Terminal bool |
There was a problem hiding this comment.
do we need this? should the status be enough to reflect whether it's terminal or not
albertywu
changed the title
| if err := store.GetRequestLogStore().Insert(ctx, log); err != nil { | ||
| return fmt.Errorf("failed to insert request log for request_id=%s: %w", log.RequestID, err) | ||
| } | ||
| _ = store.GetRequestSummaryStore().UpsertFromLog(ctx, log) |
There was a problem hiding this comment.
let's not swallow errors
| completed_at_ms BIGINT NOT NULL, | ||
| terminal BOOLEAN NOT NULL, | ||
| version BIGINT NOT NULL, | ||
| PRIMARY KEY (queue, request_id) |
There was a problem hiding this comment.
what queries will be executed on this table?
if we filter by queue and status (i.e. executing or completed) then it will do partial table scan which is inefficient
| EndTimeMs int64 | ||
| Statuses []entity.RequestStatus | ||
| Sort RequestSummarySort | ||
| Cursor *RequestSummaryCursor |
| Sort RequestSummarySort | ||
| Cursor *RequestSummaryCursor | ||
| Limit int | ||
| } |
There was a problem hiding this comment.
ok now I see a list of criteria. If we intend to keep request log records for each queue indefinitely, the index has to be redesigned. Right now it will do full scan for pretty much all of the query parameters.
If we always sort by the same field, it should be included in the index to at least do paginated index scan.
| // RequestSummaryListOptions defines a page-in request for RequestSummaryStore. | ||
| type RequestSummaryListOptions struct { | ||
| Queue string | ||
| StartTimeMs int64 |
There was a problem hiding this comment.
please comment specification, what values to set to what result to expect. How to specify that filtering should not be done by a particular field, or should it always specify arbitrary big/small values?
albertywu
force-pushed
the
wua/list-api-request-log
branch
from
5fb143c to
bf22c1a
Compare
albertywu
force-pushed
the
wua/list-api-summary-store
branch
from
788ef8d to
48dfb0b
Compare
albertywu
changed the title
albertywu
force-pushed
the
wua/list-api-summary-store
branch
from
48dfb0b to
c0e7f96
Compare
albertywu
force-pushed
the
wua/list-api-request-log
branch
from
bf22c1a to
78d7442
Compare
3 participants
Summary
Adds the RequestSummary entity, storage contract, generated mocks, and MySQL implementation for the gateway-owned list read model. The store merges immutable request-log events into a queue-scoped summary table with optimistic concurrency.
Test Plan
✅
make fmt && make build && make test && make check-mocks && make e2e-testIssues
Stack