Plan #17: ingestion API client - #66
craigmcchesney wants to merge 2 commits into
Conversation
Triage of the AI-drafted ticket against dp-service origin/main (7e8b2e6) and dp-grpc e775244. The payload model it scopes as Phase 1 already exists (#6's data_frame.py). An ack is not success, so queryRequestStatus moves into scope. The plan also covers streaming semantics that differ from the proto comments, grpcio swallowing request-iterator exceptions, the non-scalar column builders, and chunking under the server's message and span caps. Two PRs: client, then live tests and docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QkZu3MPfUopTisahmvhakD
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The plan has unresolved contract and design gaps in error results, polling, chunk sizing, and column round-tripping.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Plan-only PR defining the ingestion API client implementation and delivery phases.
Changes:
- Documents ingestion and asynchronous status behavior.
- Defines streaming, polling, column builders, and chunking.
- Splits implementation into client/tests and integration/docs work.
| File | Description |
|---|---|
plan/tickets/17/plan.md |
Ingestion API design, decisions, tests, and implementation plan. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - At least one limit is required. `max_bytes` is measured with protobuf's `ByteSize()` on the chunk | ||
| *plus* a fixed allowance for the request envelope, so a chunk that fits the budget fits the message. A | ||
| single row larger than `max_bytes` raises, naming the row. |
There was a problem hiding this comment.
Agreed; fixed in 6a9829d. The envelope is exactly providerId + clientRequestId + frame, so D8 now budgets the whole serialized IngestDataRequest: overhead is ByteSize() of a request holding the chunk and two worst-case (256-char, UTF-8) ids, and IngestDataRequestParams rejects longer ids, so nothing the params accept can exceed the budget.
| - **D9 — Size errors point at the cap.** A `RESOURCE_EXHAUSTED` from any ingest call gets a message | ||
| naming the server's inbound message limit and `split_data_frame()`, rather than grpcio's bare text. This | ||
| is additive to the `"gRPC error: ..."` contract, so it is appended, not substituted. |
There was a problem hiding this comment.
Agreed; fixed in 6a9829d. D9 now gates the hint on the status details matching a known size-violation text (grpc-java server inbound limit, grpcio client receive limit); any other RESOURCE_EXHAUSTED keeps the plain "gRPC error: ..." message.
- D6: await_request_statuses() polls with one provider + time-range query and matches ids client-side (RequestIdCriterion holds one id and criteria AND). A required `since` floor bounds the unpaged response and keeps stale documents for reused ids from satisfying the wait (Q5). - T3: record the one-id criterion and the server skipping blank criteria; the RS helpers' blank rejection is load-bearing. - D8: max_bytes budgets the whole IngestDataRequest, with ids capped at 256 chars by IngestDataRequestParams. - D9: gate the RESOURCE_EXHAUSTED hint on size-violation details. - Out of scope: queryRequestStatus paging, to be filed upstream. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QkZu3MPfUopTisahmvhakD
Refs #17
Plan-only PR:
plan/tickets/17/plan.md, the triage and implementation plan for the ingestion client. Implementation follows in two later PRs (client + unit tests, then live tests + docs).Main triage findings (details and dp-service citations are in the plan's Background section):
data_frame.py).providerIdis acked and then fails asynchronously, which contradicts the proto comments. SoqueryRequestStatusand anawait_request_statuses()poller are now in scope.ingestDataStreamreportsrejectedRequestIdson an error response, so that result must keep the response.data_frame()lets through hand-built columns the server rejects. The new checks will require fixing three existing unit tests, which the plan names.Q1–Q4 were resolved before this PR. The plan's closing note lists five decisions taken without a question that are worth a look in review: D2, D4, D8, D10, and T8.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QkZu3MPfUopTisahmvhakD