Skip to content

[default values] Gate column-default rewrites on every branch, not only main - #794

Open
cbb330 wants to merge 1 commit into
chbush/read-bridge-spark-itestfrom
chbush/type2-rewrite-gate-new-snapshots-dpxm8
Open

cbb330 wants to merge 1 commit into
chbush/read-bridge-spark-itestfrom
chbush/type2-rewrite-gate-new-snapshots-dpxm8

Conversation

@cbb330

@cbb330 cbb330 commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #693. For a table with column defaults, Type 2 requires an overwrite or replace commit to send a matching initial-default, so only default-aware clients can rewrite it. The check looked only at the snapshot main points to. An unaware client could still rewrite the table on a named branch or as a staged WAP snapshot, and creating a branch or tag at an existing overwrite was rejected as a rewrite.

The snapshots PUT already carries the table's full snapshot list. This change gates the overwrite and replace snapshots in that list that the table has not persisted yet, whichever ref they are on. It uses the existing request fields, so there is no API change. It compares snapshot ids, as the commit path already does to find new snapshots, and does not diff ref maps, which #693 rules out.

On #681, below the client gate (#774), which merges last. The code does not depend on the PRs below it; the stack keeps the column-default changes in one merge order.

Changes

  • Client-facing API Changes

  • Internal API Changes

  • Bug Fixes

  • New Features

  • Performance Improvements

  • Code Style

  • Refactoring

  • Documentation

  • Tests

  • Rewrite detection: ReadBridgeStripProtection treats a commit as a rewrite when it is a replace, or when jsonSnapshots contains an overwrite or replace snapshot the table has not persisted. Snapshots already on the table are history, so ref-only commits (create a branch or tag, roll back to an existing snapshot) are not gated.

  • Persisted snapshot ids: the new OpenHouseInternalRepository.findSnapshotIds, wired in ApiConfig, returns every snapshot id in the table's current metadata, including staged snapshots no ref points to. It is read only for a stamped, ramped table whose PUT lists an overwrite or replace snapshot, including older ones still in history. Those commits cost one extra metadata load; tables without column defaults pay nothing.

  • Failure: if the lookup fails, the commit fails closed with the lookup's own exception. The request is not at fault, so it is not reported as a 400; the exception advice handles it like any other catalog failure.

Commit on a table with column defaults Before After
Overwrite on a named branch accepted needs a matching initial-default
Staged WAP overwrite (no ref yet) accepted needs a matching initial-default
Overwrite on main needs a matching initial-default unchanged
Branch or tag created at an existing overwrite rejected accepted
Append with an older overwrite in history accepted unchanged

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.

  • Added new tests for the changes made.

  • Updated existing tests to reflect the changes made.

  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.

  • Some other form of testing like staging or soak time in production. Please explain.

  • ReadBridgeStripProtectionTest: a branch overwrite next to a main append and a staged WAP overwrite are rejected; a branch and tag at a persisted overwrite, and a historical overwrite under a current append, are accepted; a failing lookup fails closed with its own exception.

  • ReadBridgeColumnDefaultE2ETest.snapshotsPut_gatesAddedBranchRewritesButNotRefsAtPersistedRewrites drives the snapshots endpoint through the real service, repository and catalog. A branch overwrite returns 400 COLUMN_DEFAULT_REWRITE, a handshake overwrite on main commits, and a branch plus tag at that overwrite commit. Without the fix, on main at 5ee734d2, the branch overwrite returned 200.

  • On this stack, with JDK 11: :services:tables:test for readbridge.*, ReadBridgeColumnDefaultE2ETest, SnapshotsControllerTest, mock.*, services.*, repository.* and config.* (686 tests) and :client:secureclient:test (20 tests) pass.

  • Rebuilt below the client gate: the end-to-end test creates its table and PUTs snapshots without the gate's Spark-client headers. :services:tables:test (923 tests) passes and spotlessCheck is clean.

  • Restacked on [default values] Fail loudly when column defaults cannot be applied #790's typed failures (6ef9c8a9): a failing lookup no longer becomes COLUMN_DEFAULT_UNUSABLE (400). ReadBridgeStripProtectionTest passes on this commit; at the top of the stack, :services:tables:test (924 tests) passes and spotlessCheck is clean.

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

Stack

Stack #798, bottom to top:

  1. #793: report the client release in the User-Agent
  2. #790: fail loudly when column defaults cannot be applied
  3. #797: make the opt-in immutable once set to true, with the policy override
  4. #681: Spark catalog itest (overlay)
  5. This PR: gate column-default rewrites on every branch
  6. #774: gate opted-in table IO on Spark client version (merges last)

@cbb330
cbb330 added this pull request to stack #795 October 2, 2026 19:15
@cbb330
cbb330 removed this pull request from stack #795 October 2, 2026 23:22
@cbb330
cbb330 force-pushed the chbush/type2-rewrite-gate-new-snapshots-dpxm8 branch from e169411 to 3112335 Compare October 2, 2026 23:22
@cbb330
cbb330 added this pull request to stack #798 October 2, 2026 23:24
@cbb330 cbb330 closed this Oct 2, 2026
@cbb330 cbb330 reopened this Oct 2, 2026
cbb330 added a commit that referenced this pull request Oct 5, 2026
…ompiled constant (#793)

## Summary

OpenHouse's Java client sends `User-Agent:
openhouse-java-client/<version>` (#636), reading the version from the
manifest of the jar that holds `WebClientFactory`. The runtime uber jars
carry no `Implementation-Version`, and jars that re-bundle the client
replace the manifest, so those clients send
`openhouse-java-client/unknown`. In production over 24 hours, 35% of
table loads and commits did. This change compiles the release into the
client instead. The column-default client gate (#774) depends on it: it
admits opted-in tables only for clients that report a release.

Bottom of the column-default stack: #790, #774, #775, #681 and #794
build on it.

## Changes

- [ ] Client-facing API Changes
- [ ] Internal API Changes
- [x] Bug Fixes
- [ ] New Features
- [ ] Performance Improvements
- [ ] Code Style
- [ ] Refactoring
- [ ] Documentation
- [ ] Tests

- `client/secureclient` generates `ClientVersion.VALUE` from
`project.version` at build time; publishing sets it with `-Pversion`.
Unversioned local builds still report `unknown`.
- `WebClientFactory` reports that constant unless `setClientVersion`, or
the catalog's `client-version` property, overrides it. A compiled
constant survives shading, relocation and re-bundling.
- The manifest lookup and the `secureclient` manifest stamp are removed.

## Testing Done

- [ ] Manually Tested on local docker setup. Please include commands
ran, and their output.
- [ ] Added new tests for the changes made.
- [ ] Updated existing tests to reflect the changes made.
- [x] No tests added or updated. Please explain why. If unsure, please
feel free to ask for help.
- [x] Some other form of testing like staging or soak time in
production. Please explain.

The failure only appears once the client is packaged, and CI builds
without `-Pversion`, so a unit test can't catch it.
`:client:secureclient:test` passes. I built the runtime uber jars with
`-Pversion=0.5.999` and sent one request through
`TablesApiClientFactory` to a local server that records the User-Agent:

| Client classes loaded from | User-Agent |
|---|---|
| published `openhouse-java-runtime-0.5.503-uber.jar` |
`openhouse-java-client/unknown` |
| this branch's `openhouse-java-runtime` uber jar |
`openhouse-java-client/0.5.999` |
| this branch's `openhouse-spark-runtime_2.12` uber jar |
`openhouse-java-client/0.5.999` |
| the same classes unpacked, with no manifest |
`openhouse-java-client/0.5.999` |
| repacked into a jar whose manifest says `9.9.9` |
`openhouse-java-client/0.5.999` |
| with `setClientVersion("4.2.150")` | `openhouse-java-client/4.2.150` |

# Additional Information

- [ ] Breaking Changes
- [ ] Deprecations
- [ ] Large PR broken into smaller PRs, and PR plan linked in the
description.

Clients that set `client-version` are unaffected. Clients that don't,
and previously had a manifest version, now report the OpenHouse release
they embed rather than the enclosing jar's version. li-openhouse sets
`client-version` to its own runtime version in
linkedin-multiproduct/li-openhouse#2420; merge that before li-openhouse
picks up this release.

<!-- 8kz0b -->
@cbb330
cbb330 force-pushed the chbush/type2-rewrite-gate-new-snapshots-dpxm8 branch 2 times, most recently from f96e658 to b80ad36 Compare October 5, 2026 22:47
@cbb330
cbb330 force-pushed the chbush/type2-rewrite-gate-new-snapshots-dpxm8 branch 2 times, most recently from a9d7a1b to fcb3b8f Compare October 6, 2026 17:12
@cbb330
cbb330 force-pushed the chbush/type2-rewrite-gate-new-snapshots-dpxm8 branch from fcb3b8f to 9ebb74b Compare October 6, 2026 19:57
@cbb330
cbb330 force-pushed the chbush/type2-rewrite-gate-new-snapshots-dpxm8 branch 2 times, most recently from c74c2c2 to 12f69c4 Compare October 6, 2026 20:26
Type 2 required the initial-default handshake only when a commit moved main
to an overwrite or replace snapshot. An unaware client could still rewrite a
stamped table on a named branch or as a staged WAP snapshot. Creating a
branch or tag at an existing overwrite was rejected as a rewrite.

The snapshots PUT carries the table's full snapshot list. Gate the overwrite
and replace snapshots in it that the table has not persisted yet, whichever
ref they are on. Persisted snapshot ids are read from the catalog only for a
stamped table whose list contains such a snapshot; a failed read fails the
commit as itself, since the request is not at fault.

Fixes #693.
@cbb330
cbb330 force-pushed the chbush/type2-rewrite-gate-new-snapshots-dpxm8 branch from 12f69c4 to 6ef9c8a Compare October 6, 2026 21:37

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Type 2 column-default rewrite gate should use commit deltas, not main-only

1 participant