Plan #17: ingestion API client - #66
Merged
Merged
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.
- 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
IngestionServiceImpl.queryRequestStatus() validates every criterion before the Mongo client runs, so its isBlank() skips are unreachable. The RS helpers' blank rejection saves a round trip; it is not load-bearing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QkZu3MPfUopTisahmvhakD
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QkZu3MPfUopTisahmvhakD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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