Conversation
patch_mixin._entity_etag recomputed the server's pre-#30497 strong ETag (SHA-256 of "<version>-<updatedAt>"), which the server now keeps only as generateETag's serialization-failure fallback. Every conditional column tag/description patch therefore failed its precondition, retried, and rewrote non-conditionally: two wasted round trips per entity and no optimistic locking, so a concurrent column add/remove/reorder could still send an index-based patch to the wrong column. Send W/"<version>" instead - the form validateETag accepts via isWeakMatch. Unlike the strong ETag it does not depend on the caller's `fields` projection, so it is the only validator a client can reproduce; it is what the row-level compare-and-swap already keys on, and it has been accepted since ETag support landed in #22291, so older servers either honour it or ignore If-Match entirely. The strong ETag remains correct for If-None-Match/304 caching and is untouched. Why it cannot serve conditional writes is now documented on EntityETag.generateWeakETag/validateETag; the server-side half is #30885. Tests pin the validator format on both sides to the same literals, so a future change to it fails a test instead of silently degrading every ingestion to last-write-wins. OptimisticLockingColumnPatchIT previously only used If-Match: *, which matches any ETag and so could not catch this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three "function called with args" lines fired at INFO on every conditional PATCH: validateETag's entry + computed-ETag lines and EntityRepository's pre-validation line (both patch call sites). A first-time classification over a 10k-table catalog emits ~30k of them. The two lines that carry signal are untouched, so nothing observable is lost: a failed precondition still logs EntityETag's "ETag mismatch" WARN, and a successful conditional write still logs store()'s "Updated ... with version check" at INFO, which runs only on the version-CAS path. Separate commit so it can be reverted independently of the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
patch_column_descriptions carries its own copy of the retry loop, so the weak-validator change lands there too but was only covered transitively. Assert the header it sends, so the two paths cannot drift apart silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Ingestion
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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| return None | ||
| raw = f"{model_str(version)}-{model_str(updated_at)}" | ||
| return '"' + hashlib.sha256(raw.encode("utf-8")).hexdigest()[:16] + '"' | ||
| return f'W/"{float(model_str(version)):.1f}"' |
There was a problem hiding this comment.
💡 Edge Case: Weak ETag format relies on versions always being one decimal
Python renders the validator with a fixed :.1f, while the Java server uses Double.toString(version). These agree only because EntityUtil.nextVersion yields exact n/10.0 values today; a version with two decimals (e.g. 1.25) or one large enough to trigger Java scientific notation (>=1e7) would render differently and silently degrade every conditional write to the last-write-wins fallback rather than fail loudly. This is not a data-loss risk (the fallback still persists the change) and both sides pin the current literals in tests, so it is defensive only — but a comment or a shared assertion tying :.1f to the nextVersion rounding invariant would prevent a future version-scheme change from silently disabling optimistic locking for all non-Java clients.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsSends the weak ETag validator 💡 Edge Case: Weak ETag format relies on versions always being one decimal📄 ingestion/src/metadata/ingestion/ometa/mixins/patch_mixin.py:103 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/EntityETag.java:108 Python renders the validator with a fixed 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
There was a problem hiding this comment.
Pull request overview
Fixes ingestion column tag/description conditional PATCHes always failing If-Match preconditions by switching the Python client’s locally-computed validator to the server-accepted weak ETag form (W/"<version>"), restoring effective optimistic locking and avoiding deterministic 412→retry→unconditional fallback behavior.
Changes:
- Update ingestion
patch_mixin._entity_etagto emit weak version validators (W/"<version>") and improve retry/fallback log messages. - Document the strong-vs-weak ETag contract in
EntityETag.validateETag/generateWeakETagand add backend tests proving weak validators work across projections where strong ones cannot. - Add ingestion unit tests and backend integration tests that pin the weak ETag wire format and validate stale-version rejection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ingestion/src/metadata/ingestion/ometa/mixins/patch_mixin.py | Switches client-side If-Match generation to weak version ETag and improves retry/fallback logging. |
| ingestion/tests/unit/ometa/test_patch_mixin_etag.py | Adds unit coverage pinning weak ETag format + retry/fallback behavior for column tag/description patch helpers. |
| openmetadata-service/src/main/java/org/openmetadata/service/util/EntityETag.java | Documents weak ETag as the required optimistic-write validator; reduces verbose INFO logs to DEBUG in validateETag. |
| openmetadata-service/src/test/java/org/openmetadata/service/util/EntityETagTest.java | Adds tests pinning weak ETag rendering and proving weak validation survives projection changes while strong cannot. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Downgrades optimistic-locking PATCH ETag-validation log lines from INFO to DEBUG. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OptimisticLockingColumnPatchIT.java | Adds integration tests using weak version If-Match to ensure persistence and stale-version rejection. |
🔴 Playwright Results — workflow failedValidated commit ✅ 607 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky Pipeline and setup failures (1)
PerformanceBlocking targets: ❌ unmet · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 4m 57s ⏱️ Max setup 3m 11s · max shard execution 18m 24s · max shard-job elapsed before upload 31m 57s · reporting 6s 🌐 208.36 requests/attempt · 2.76 app boots/UI scenario · 4.26% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
Describe your changes:
Fixes #30886
patch_mixin._entity_etagrecomputed the server's pre-#30497 strong ETag locally —SHA-256("<version>-<updatedAt>")— whichEntityETag.generateETagnow keeps only as its serialization-failure fallback. So every conditional column tag/description patch failed its precondition, retried, and then rewrote non-conditionally: two wasted HTTP round trips per entity plus a WARN on both sides, and the optimistic locking the header exists for was silently absent, leaving the wrong-column hazard live. This sendsW/"<version>"instead — the validatorvalidateETagaccepts viaisWeakMatch— which, unlike the strong ETag, is independent of the caller'sfieldsprojection and so is the only validator a client can reproduce.Type of change:
High-level design:
Why the strong ETag cannot work for writes.
ETagResponseFilterhashes the response entity — the caller'sfieldsprojection.EntityRepository.patchvalidatesIf-Matchagainstoriginal = get(null, id, patchFields, …)— the repository's projection (TableRepository.PATCH_FIELDS=tableConstraints,tablePartition,columns+ tags). Same entity, same version, different bytes, different hash. No client can satisfy it, however faithfully it stores the ETag.Why weak is the right validator. A cache validator must vary with the response body (that's #30497, correct and untouched here); a write validator must not vary with the projection. One ETag can't do both — which is what HTTP's strong/weak distinction is for.
W/"<version>"is projection-independent, is what the row-level CAS instore→EntityDAO.updateWithVersionalready keys on, and has been accepted since #22291, so older servers either honour it or ignoreIf-Matchentirely.EntityUtil.nextVersionrounds to one decimal place, which is what lets Python reproduce the JavaDoublerendering.Alternative rejected: echoing the ETag from the client's own GET — the textbook-correct approach — still 412s for the projection reason above, and would need
raw=Trueplumbing through the shared REST client to reach the header. The server-side contract fix that would make it viable is filed separately as #30885; this PR is deliberately client-side plus documentation, with no change to server behaviour.Scope note: the three server-side
LOG.info→LOG.debugdowngrades are an isolated second commit (fefd883c41), revertable without touching the fix. The signal-bearing lines are untouched: a failed precondition still WARNs, and a successful conditional write still logsstore()'s "Updated … with version check" at INFO.Tests:
Use cases covered
Unit tests
ingestion/tests/unit/ometa/test_patch_mixin_etag.py(11 tests) — pins the wire format across0.1 / 0.4 / 1.0 / 2.3 / 10.0, plus the retry/fallback cycleEntityETagTest.weakETagIsTheContractForNonJavaClients— asserts the same literals as the Python test, so the two sides are pinned independently and a rename on either fails a testEntityETagTest.weakETagValidatesAcrossProjectionsWhereStrongETagCannot— encodes the root cause: same entity + version under two projections, strong throwsPreconditionFailedException, weak validatespytest … -q→ 11 passed; the same tests run against the pre-fix implementation → 9 failed (genuine RED→GREEN).mvn test -pl openmetadata-service -Dtest=EntityETagTest→ 11 tests, 0 failurespatch_mixin.py18% → 25% module-wide from these tests. The module figure is low because the file holds ~30 unrelated patch helpers exercised only by integration tests; every branch this PR changes on the tag path is unit-covered, and on the description path the conditional-write branch is unit-covered with its 412 branches covered by the pre-existingtest_patch_column_falls_back_when_etag_unusableBackend integration tests
openmetadata-integration-tests/.OptimisticLockingColumnPatchIT:patchWithWeakVersionIfMatch_columnTag_persistsandpatchWithStaleWeakVersionIfMatch_isRejected. Every pre-existingIf-Matchtest in that class used*, which matches any ETag — so they cover the Optimistic-locking (If-Match) PATCH silently drops nested column tag/description changes #28876 updater delegation but structurally cannot catch a validator mismatch. These build the header the way the client does. Compiles clean (mvn test-compile); they need a live stack, so they first execute in CI.Ingestion integration tests
tests/integration/ometa/test_ometa_patch.py::test_patch_column_tags_retries_on_concurrent_modificationwas previously vacuous (it injected a concurrent writer, but everyIf-Match412'd regardless, so it could not tell the injected bump from the formula mismatch) and now tests what its name says.Playwright (UI) tests
If-Match;etagInterceptor.tsonly issuesIf-None-Matchconditional GETs.Manual testing performed
Diagnosed from an Auto Classification agent run against a Redshift service (29 tables): 14 assets had a tag to write, and all 14 logged the 412-then-fallback pair with zero conditional writes succeeding — the 1:1 ratio is what identified this as a deterministic formula mismatch rather than contention. Post-fix behaviour is asserted by the tests above rather than re-run by hand;
mvn spotless:checkandruff check/ruff formatare clean on all changed files.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.🤖 Generated with Claude Code