Repository navigation
Fixes 35909: report bulk-create outcomes and recover conflicts - #35910
Conversation
Centralize createMany recovery in EntityRepository. Skip existing and repeated FQNs without overwriting or restoring entities, and return only newly created entities in request order. After a concurrent duplicate rolls back a transaction, warn and retry only the remaining entries in that 100-row chunk. Finish creation hooks for each committed chunk and propagate unrelated failures. Preserve single-create and bulk-upsert behavior. Add real database regressions for test cases and glossary terms, concurrent conflicts, later chunks, soft deletes, and error propagation. Document the API contract and decision. Verified 23 unit tests and 21 integration tests on each of PostgreSQL and MySQL.
Recheck the exact FQN when preparation rejects a bulk-create entry. Skip a concurrently created entity while preserving validation errors for different FQNs and validating all remaining entries before writes. Add a deterministic glossary preparation race regression and a different-parent name validation regression. Extend the decision record to cover preparation recovery.
github-actions
Bot
added
backend
safe to test
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Share the existing-FQN lookup across filtering and conflict recovery, prepare candidates through a per-entity predicate, and use the existing partition utility. Remove the temporary mutable singleton list and manual chunk indexing while preserving validation, retry bounds, and committed-ID protection.
Return created, skipped, and failed counts with ordered reasons and stored IDs. Keep skip as the default and allow fail to mark conflicts as item failures while creating other valid inputs. Move shared preparation and duplicate recovery into BulkEntityCreator, regenerate the public models, and update both REST endpoints, SDK services, and database regressions.
harshach
changed the title
Jest test CoverageUI tests summary
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @classmethod | ||
| def _create_many(cls, requests: Sequence[TCreate], on_conflict: ConflictPolicy) -> BulkCreateResult: | ||
| client = cls._get_client() | ||
| endpoint = cls._get_endpoint_path(client) | ||
| # REST drops empty json lists; data preserves an empty batch as []. | ||
| payload = json.dumps( | ||
| [request.model_dump(mode="json", by_alias=True, exclude_none=True) for request in requests] | ||
| ) | ||
| response = cls._get_rest_client(client).post( | ||
| f"{endpoint}/createMany?onConflict={on_conflict.value}", data=payload | ||
| ) | ||
| return BulkCreateResult.model_validate(response) |
There was a problem hiding this comment.
⚠️ Bug: Python create_many retries non-idempotent POST on 503/504
_create_many calls rest_client.post(...) with the client's default retry budget (retry=3, retry_codes=[503, 504], see ingestion/src/metadata/ingestion/ometa/client.py:186-189,386). Suppose a proxy returns 504 after the server has already committed some chunks. The SDK then sends the same batch again, and the retry reports every entity the first attempt created as skipped/ALREADY_EXISTS (or failed under fail) with no IDs. The caller gets a result that says nothing was created. Separately, if the retries run out or the catch-all in _one_request swallows an error, _request returns None and BulkCreateResult.model_validate(None) raises a pydantic ValidationError instead of APIError. The fix is to pass retries=0 for this POST and to raise a clear error when the response is empty.
Disable automatic retries for the non-idempotent bulk POST and fail clearly on an empty response:
response = cls._get_rest_client(client).post(
f"{endpoint}/createMany?onConflict={on_conflict.value}", data=payload, retries=0
)
if response is None:
raise RuntimeError(f"No response from {endpoint}/createMany")
return BulkCreateResult.model_validate(response)
✅ Fixed by @harshach in c2824d2
Reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Fixed in c2824d2. The shared Python bulk-create helper now sends the POST with retries=0. A missing response raises a clear RuntimeError naming the endpoint and noting that creation may have completed, rather than a Pydantic validation error. HTTP errors still propagate.
Added 14 regression cases through the real REST client for both facades: 503/504 are not replayed under either conflict policy, and empty/null responses or swallowed client errors fail clearly. All 14 failed before the fix and now pass. Full Python SDK suite: 486 passed, 1 existing skip.
| /** Bulk creation with per-input outcomes. Defaults to skipping conflicts. */ | ||
| public final class BulkCreateRequest { | ||
| private final Function<BulkCreateConflictPolicy, BulkCreateResult> createFunction; | ||
| private BulkCreateConflictPolicy onConflict = BulkCreateConflictPolicy.SKIP; |
There was a problem hiding this comment.
⚠️ Quality: Fluent/Python SDK default SKIP + forced detailed mode not in ADR
This commit adds a new default for SDK callers. TestCases.createMany(...) and GlossaryTerms.createMany(...) (Java fluent) and create_many() (Python) always send onConflict=skip unless the caller chooses otherwise. Because a policy is always sent, they always get the detailed response and need a server that supports onConflict (READMEs: "Both builders default to SKIP and explicitly request detailed outcomes… These methods require a server that supports onConflict"). The ADR 2026-10-21-bulk-test-case-creation-skips-existing-fqns.md (lines 79-83) only records the Java service-level overloads. It says nothing about the fluent/Python facades, their SKIP default, or that they don't work with older servers. The record should say: "The Java fluent and Python SDK bulk-create facades default to onConflict=skip, always request the detailed BulkCreateResult, and are unsupported against servers without onConflict."
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Fixed in c2824d2: appended Amendment v5. It explicitly records that both Java fluent and Python bulk-create facades default to onConflict=skip, always request detailed BulkCreateResult outcomes, and are unsupported against servers without onConflict.
The amendment distinguishes the existing Java service overload's array response and records the Python REST retry policy. No Java runtime change was needed.
Code Review ✅ Approved 2 closed / 2 findings🔴 High risk · Changing shared EntityRepository bulk-create conflict recovery affects persisted entity writes; faulty duplicate detection could skip valid entities or misreport creations. Adds per-input outcome reporting and conflict recovery to bulk create so unrelated new entities are no longer rolled back when one FQN already exists, with explicit ✅ 2 closed✅ Bug: Python create_many retries non-idempotent POST on 503/504
✅ Quality: Fluent/Python SDK default SKIP + forced detailed mode not in ADR
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
|
createMany mapped and stored the terms without any authorization check, so a caller without CREATE could bulk-create terms in any glossary. Each term is now checked for limits and CREATE before anything is written, as in main (#35910).
Describe your changes:
Fixes #35909
Bulk creation could roll back unrelated new entities when one FQN already existed. Recover those conflicts centrally, preserve existing entities, and let callers request an explicit outcome for every input.
For test cases and glossary terms,
POST .../createMany?onConflict=skip|failreturnscreatedCount,skippedCount,failedCount, and orderedresults. Each result has an inputindex,status, resolvable FQN, and a reason/message for skips or failures. IDs are returned only for created entities.skip: conflicts are skipped; other valid inputs are created.fail: conflicts are reported as failed; other valid inputs are still created. This is not an atomic request-wide failure policy.Python now exposes
TestCases.create_many(requests, on_conflict=ConflictPolicy.skip)andGlossaryTerms.create_many(...). Java exposesTestCases.createMany(requests).onConflict(BulkCreateConflictPolicy.FAIL).execute()and the equivalent glossary builder. Both default to skip and return the generated typed result, including reasons. New fluent calls require a server supportingonConflict.Type of change:
High-level design:
EntityRepositorydelegates to a small sharedBulkEntityCreator. It maps inputs, checks existing FQNs in a batch, validates new candidates, and creates transaction chunks of at most 100 entities through the existing repository lifecycle. The first valid occurrence of each new FQN wins. Detailed responses isolate expected mapping/preparation validation failures; legacy responses retain their original failure behavior.A concurrent winner can appear during preparation or insertion. Recovery confirms the exact FQN belongs to a different stored ID, removes that candidate, and retries only the failed transaction's remaining entries after rollback. Every retry removes an entry. Unrelated constraints, connectivity errors, and failures after our own rows commit still propagate. Each committed chunk completes relationships, indexing, and creation hooks before the next chunk starts.
Reason codes are
ALREADY_EXISTS,DUPLICATE_IN_REQUEST, andVALIDATION_ERROR. Existing and soft-deleted entities are neither overwritten nor restored. Authorization and request limits remain HTTP errors. Glossary creation now enforces the normal CREATE permission before conflict lookup; conflict results do not expose stored IDs to callers without read permission.Explicit policy selection also selects the detailed response, preserving the live Collate AI client's array parser without an extra request option or coordinated rollout. Single-create and bulk create-or-update keep their existing contracts. The new JSON schema describes a response only, so no database migration is required. Java, Python, and TypeScript models are generated from it.
See ADR:2026-10-21-bulk-test-case-creation-skips-existing-fqns.
Performance: normal creation adds one batched FQN lookup and O(N) bookkeeping. Reporting adds no entity fetches. Conflicts can add lookups/retries; multi-chunk requests complete search refreshes per chunk. No latency benchmark was run.
The Python facades share a small serialization helper; the Java facades share one request builder and reuse the service methods. Python sends an empty batch as
[]instead of letting the REST client's empty-JSON handling drop the body. SDK tests now use the parent's JUnit version; the previous SDK override mixed JUnit 6.1.3 with managed Platform 1.11.4 and prevented test discovery.Tests:
Use cases covered
Unit tests
IntegrityConstraintMapperTest: 12 passed.TestCaseRepositoryTest: 11 passed.CreateEntityToolTest24 passed,CreateTestCaseToolTest4 passed,PatchEntityToolTest31 passed.BulkCreateFluentAPITestcases.Module structure test not relevant for SDK); includes all 10 newtest_bulk_create.pycases. Scoped Ruff and basedpyright checks passed.a2ac1594dbef2f5460a85b83875967f84de25536.Review follow-up at
c2824d23cc08c5add4e5b02c8e5bc86c819d7850(Python SDK and ADR only):PYTHONPATH=ingestion/src python -m pytest -q ingestion/tests/unit/sdk --cov=metadata.sdk.entities.base --cov-report=term: 486 passed, 1 existing skip, including 24 bulk-create cases. The 14 added cases reproduced failures before the fix._create_manyhelper has 7/7 lines covered; the completebase.pyremains 89.1% (279/313).basedpyright src/metadata/sdk/entities/base.py, decision-record validation,git diff --check, and commit hooks passed. An additional type-check attempt including the test module reported 34 errors in its unchanged fixtures/assertions; tests are outside the configuredinclude = ["src"]type-check scope.a2ac1594dbef2f5460a85b83875967f84de25536.JaCoCo from the targeted MySQL run:
BulkEntityCreator91/91 lines (100%) and 34/36 branches (94.4%); its candidate record 2/2 lines. The unexercised branches guard against treating our own stored ID as a concurrent winner. Whole-class line coverage isEntityRepository46.5% (2727/5870),TestCaseResource14.9% (58/388), andGlossaryTermResource14.4% (40/277). The existing large classes remain below the repository's 90% whole-class target.SDK coverage:
BulkCreateRequest7/7 lines, both new JavacreateManymethods 3/3 lines, and all new Python bulk-create methods 100%. Whole existing Python files:base.py89%,glossaryterms.py95%,testcases.py100%; whole Java facade classes:GlossaryTerms69.2%,TestCases38.9%. These existing whole-class gaps are not presented as meeting the 90% target.Backend integration tests
TestCaseResourceIT,GlossaryTermResourceIT, andEntityBulkCreateRecoveryIT.40 passed on PostgreSQL + OpenSearch and 40 passed on MySQL + Elasticsearch, with no failures or skips, on reviewed commit
a2ac1594dbef2f5460a85b83875967f84de25536. Both Maven builds succeeded.TestCaseResourceITGlossaryTermResourceITEntityBulkCreateRecoveryITTableResourceITAlertStateITBefore adding the fluent methods, the 10 Python cases failed with missing
create_many, and the Java test failed compilation on the missing bulk request type. After implementation and JUnit dependency alignment, both full SDK suites passed.The initial detailed-response RED run failed both new cases against the old array/validation behavior. The first-valid preparation regression also failed before its fix (zero created instead of one). An earlier 35-case run had one glossary fixture-name collision between parameterized cases; the fixture now uses distinct names. The earlier preparation-race regression also demonstrated the original HTTP 400 failure. Final results below supersede those development runs.
Targeted verification commands
The MySQL run also uses JaCoCo 0.8.13 for
BulkEntityCreator,EntityRepository,TestCaseResource, andGlossaryTermResource. The full SDK run uses JaCoCo 0.8.13 for the fluent package.mvn spotless:apply,mvn spotless:check, scoped Ruff/basedpyright,git diff --check, the impact-map branch check, and commit hooks passed on the SDK follow-up. Earliermake generate, canonical TypeScript generation, Python result-model validation, and generated OpenAPI checks passed on the unchanged schemas. Generated TypeScript is excluded from the repository's handwritten UI lint/format checks.PENDING. The targeted results above do not replace the full affected-suite selection.
make java_affectedselects the full service unit suite, full merge-queue integration suites, cache/PostgreSQL lanes, and 28 PostgreSQL/OpenSearchsearch-itclasses. No generated passing affected-suite report is claimed.Full CI runs on
a2ac1594dbef2f5460a85b83875967f84de25536:These four runs are still in progress as of this update. The 28 selected
search-itclasses have not been run locally, and the complete affected-suite report and whole-class coverage target remain outstanding. UI and nightly scale suites were not run because this change has no UI behavior or scale-fixture changes.Ingestion integration tests
Python SDK changes are covered by the full SDK unit suite, with the real REST serializer and a mocked HTTP transport. No connector changes. A live-server Python SDK integration run was not performed; Java API integration tests exercise the matching endpoint contract on both databases.
Playwright (UI) tests
Not applicable — generated type only; no UI behavior changed.
Not applicable — no UI behavior changes.
Manual testing performed
No interactive manual testing. Integration tests start real database/search containers and verify HTTP responses, database state, relationships, rollback, and search visibility.
UI screen recording:
Not applicable — backend/API change with no UI behavior changes.
Checklist:
Fixes 35909: report bulk-create outcomes and recover conflicts.Fixes #35909above.c2824d23cc08c5add4e5b02c8e5bc86c819d7850confirmed both requested Python/ADR fixes. One additional Java SDK retry finding remains, described below.Remaining validation: finish the full affected-test selection and record its generated report; assess the whole-class coverage gap. Targeted tests and review do not replace those gates.
Additional merge risk found during this review (not changed in this Python/ADR follow-up): the Java detailed bulk-create calls in
TestCaseServiceandGlossaryTermServiceuse default OkHttp replay behavior. A local-server reproduction throughTestCases.createMany(...).execute()returned503withRetry-After: 0, followed by a conflict response: the SDK sent 2 requests and returned created=0, skipped=1. These Java calls need replay prevention and a regression test before merge. This is separate from the two requested review threads; no remaining-risk or full-PR approval is implied by addressing them.
The reviewed changes appear safe to merge; no new blocking code issues were found.Summary
Bulk creation now reports ordered outcomes, preserves existing entities, and recovers concurrent conflicts without dropping unrelated new inputs.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Bulk create request] --> B[Map inputs and check existing names] B --> C[Prepare new candidates] C --> D[Create transaction chunk] D -->|Success| E[Complete creation hooks] D -->|Duplicate key| F[Confirm another stored ID] F -->|Conflict found| G[Remove conflicting inputs] G --> D F -->|No matching conflict| H[Propagate error] E --> I[Return legacy array or detailed outcomes]Reviews (5) · Last reviewed commit: "Prevent automatic retries of Python bulk..." · Reviewed by Greptile