Fold Git-for-data isolate into OpenHouseSparkCatalog - #11
Draft
cbb330 wants to merge 76 commits into
Draft
Conversation
) ## Summary `OpenHouseDataLayoutStrategyGenerator` invokes several Spark terminal actions (`count`, `collectAsList`, `toLocalIterator`) on the lazy `Dataset<FileStat>` returned by `tableFileStats.get()`. Without caching, every action independently materializes the Dataset by re-reading the underlying Iceberg `system.files` manifest. In `generateTableLevelStrategies`, that's 5-7 manifest reads per app run; in `generatePartitionLevelStrategies` it's that figure multiplied by partition count, since the loop calls `tableFileStats.get()` fresh per partition. This PR adds `.cache()` so the Dataset is materialized once and subsequent actions read from the cached partitions instead of re-reading the manifest. The action count is unchanged - this only makes each action faster by eliminating the redundant scan work. A try/finally `unpersist()` bounds the cache lifetime to the generator invocation so the cached blocks are released promptly. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [x] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests ### Details In `OpenHouseDataLayoutStrategyGenerator.java`: - `generateTableLevelStrategies`: wrap `tableFileStats.get()` with `.cache()`, add `try/finally unpersist()`. - `generatePartitionLevelStrategies`: cache the unfiltered Dataset **outside** the per-partition loop; each per-partition filter now derives from the cached Dataset rather than calling `tableFileStats.get()` again. No other logic changed. No API change. ## 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. - [ ] Some other form of testing like staging or soak time in production. Please explain. The change is a transparent performance hint - the public API and every observable output (every `DataLayoutStrategy` field) is unchanged. Verified by running the existing suite: - `./gradlew :libs:datalayout:test` - all 17 tests pass, including: - `OpenHouseDataLayoutStrategyGeneratorTest.testTableLevelStrategy` - `OpenHouseDataLayoutStrategyGeneratorTest.testTableLevelStrategyPartitioned` - `OpenHouseDataLayoutStrategyGeneratorTest.testPartitionLevelStrategy` - `IntegrationTest.testCompactionStrategyGenerationNonPartitioned` - `IntegrationTest.testCompactionStrategyGenerationWithPersistencePartitioned` No new tests added because there is no new behavior to assert. The win is reduced per-action wall time, which only manifests against a real Spark cluster. ## Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description.
## Summary Introduces the OpenTelemetry metrics scaffolding for the Python dataloader. No instruments are emitted yet — this PR only adds the plumbing so future PRs can register metric groups without touching dependency, wiring, or test-harness code. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - Adds `openhouse.dataloader.metrics` package with `DataLoaderMetrics` (holds a shared OTEL `Meter`) and `get_metrics()` (returns the default or a custom-provider instance). - Adds `opentelemetry-api` as a runtime dependency (no-op until an SDK is configured by the consumer) and `opentelemetry-sdk` as a dev dependency. - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. - Follow-up PRs will introduce concrete instrument groups (e.g. split iteration metrics) on top of this scaffolding.
) ## Summary Adds an `id` property to `OpenHouseDataLoader`. Each instance gets a unique `dataloader-<uuid>` id at construction, useful for logging and correlation. ## Changes - [x] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests ## Testing Done - [x] Added new tests for the changes made. - [ ] Updated existing tests to reflect the changes made. - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [ ] 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. `make verify` passes (lint, format, mypy, 264 pytest). # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description.
…din#509) ## Design Doc Summary Full design doc: https://docs.google.com/document/d/1PjuXS4xER-oA0Q06e-qUvkrh7tSfhj5h873f7nq1rEg ### Problem Kafka ETL reported that OpenHouse `PUT /snapshots` (commit) p99 latency is missing SLOs, sometimes exceeding 1 minute. Root cause: `refreshMetadata` is called **6 times** per commit request, but **3 of those are redundant real HDFS reads** of the same pre-commit metadata file. This happens because each `loadTable()` creates a fresh `TableOperations` instance with no shared cache. The 6 refreshes break down as: | # | Source | Real HDFS read? | |---|--------|----------------| | 1 | `TablesService.putTable` initial `findById` | Yes | | 2 | `save()` redundant `existsById` | Yes | | 3 | `save()` real `catalog.loadTable` | Yes | | 4-5 | Transaction internal `ops.refresh()` | No (cheap no-ops) | | 6 | Post-commit response building | Yes (new metadata) | ### Solution: TableMetadata Cache - **7-day fleet analysis:** 33-38M metadata refreshes/day; ~28% are duplicates within a 10-minute window - A `TableMetadata` cache keyed by metadata location eliminates redundant HDFS reads across `loadTable()` calls within the same pod ### Cache Configuration | Property | Env var | Type | Default | |---|---|---|---| | `cluster.iceberg.tables.metadata-cache.enabled` | `CLUSTER_ICEBERG_TABLES_METADATACACHE_ENABLED` | bool | `false` | | `cluster.iceberg.tables.metadata-cache.max-weight` | `CLUSTER_ICEBERG_TABLES_METADATACACHE_MAXWEIGHT` | DataSize (`2GB`, `512MB`) | `512MB` | | `cluster.iceberg.tables.metadata-cache.ttl` | `CLUSTER_ICEBERG_TABLES_METADATACACHE_TTL` | Duration (`10m`, `30s`) | `10m` | Default-off rollout: deploy fleet-wide, flip `CLUSTER_ICEBERG_TABLES_METADATACACHE_ENABLED=true` on a canary pod via its env, watch the cache metrics, then promote the value into `cluster.yaml` for the cluster. When `enabled=false`, the bean returns Spring's `NoOpCacheManager` so `@Cacheable` methods always pass through. `max-weight` defaults to `512MB` to fit the 1GB JVM heap used in lower envs. Prod (4GB heap) can opt up to `2GB` via its `cluster.yaml`. ### Cache Metrics | Metric | Source | What it tells you | |---|---|---| | `cache_gets_total{result="hit"\|"miss"}` | auto | Hit ratio = hit / (hit+miss) | | `cache_evictions_total` | auto | Eviction rate (cause unknown) | | `cache_eviction_weight_total` | auto | Bytes evicted — confirms weigher is live | | `cache_size` | auto | Current weight in bytes | | `cache_load_duration_seconds` | auto | Refresh latency on miss | | `cache_load_total{result="success"\|"failure"}` | auto | Refresh failure rate | | `metadata_cache_removal_total{cause}` | custom | Splits evictions by `SIZE` / `EXPIRED` / `REPLACED` / `EXPLICIT` / `COLLECTED` — tells you *why* to resize | ### Before/After Validation - Traces are structurally identical (42 spans) — the cache only reduces latency within spans by avoiding redundant HDFS I/O - AFTER eliminates 8 HDFS connection DEBUG lines (24 → 16) - Single-sample latency dropped from 1643ms → 560ms; production-scale validation expected to show larger gains due to HDFS p90 variance ## Changes - [ ] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [x] Performance Improvements - [ ] Code Style - [x] Refactoring - [ ] Documentation - [x] Tests For all the boxes checked, please include additional details of the changes made in this pull request. Internal API Changes: - Introduce `TableMetadataCache` interface and `SpringTableMetadataCache` implementation; wire a Spring `CacheManager` bean (`internalCatalogCacheManager`) backed by Caffeine. - Bound the cache by byte weight (Caffeine `Weigher` over `TableMetadataParser.toJson` length) instead of entry count, exposed via `cluster.iceberg.tables.metadata-cache.max-weight`. Performance Improvements: - Eliminates redundant HDFS `TableMetadataParser.read` calls across `loadTable()` instances within a pod. Refactoring: - Add `RemovalListener` that emits a Micrometer counter tagged by `RemovalCause`, complementing Spring Boot's auto-instrumented `cache_*` metrics. - Gate the whole cache on `metadata-cache.enabled`; when off, the bean is a `NoOpCacheManager` and no Caffeine resources are allocated. Tests: - New: `CacheConfigurationTest` covers default-disabled, enabled-with-overrides, and the `@Cacheable` round-trip; `InternalCatalogBeansTest` covers property propagation and the cross-module bean wiring for both modes. ## Testing Done - [x] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] Added new tests for the changes made. - [x] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. Manually tested on local docker setup (`infra/recipes/docker-compose/oh-only`): - Brought up compose, hit `/actuator/prometheus` on the tables service: all seven cache metrics in the table above appeared with `cache="tableMetadata"` tags. - Created/read tables under a tightened `max-weight=4KB`; observed `cache_evictions_total = metadata_cache_removal_total{cause="SIZE"} = 12`, confirming the listener fires with the correct cause tag for every eviction. Validation run: `JAVA_HOME=$(/usr/libexec/java_home -v 11) ./gradlew :iceberg:openhouse:internalcatalog:test :services:tables:test` # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request.
…#582) ## Summary DataLoader reads have no built-in observability today, so callers cannot track read latency, retries, or errors. This PR emits OpenTelemetry metrics from the DataLoader so operators can monitor table-reading workloads. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request.
## Summary
Loosens the floor on `opentelemetry-api` / `opentelemetry-sdk` from
`>=1.41.1` to `>=1.38.0` in the `openhouse-dataloader` package. The
current pin demands a release line that is too new for many consumer
environments.
## Changes
- [ ] Client-facing API Changes
- [ ] Internal API Changes
- [x] Bug Fixes
- [ ] New Features
- [ ] Performance Improvements
- [ ] Code Style
- [ ] Refactoring
- [ ] Documentation
- [ ] Tests
## 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.
- [ ] Some other form of testing like staging or soak time in
production. Please explain.
Dependency-floor change only — no behavioral change. Validated with
`make verify` (ruff lint + format check, mypy, 280 pytest tests) against
the regenerated lockfile resolving `opentelemetry-{api,sdk}` to 1.38.0;
all green.
# Additional Information
- [ ] Breaking Changes
- [ ] Deprecations
- [ ] Large PR broken into smaller PRs, and PR plan linked in the
description.
…inkedin#527) ## Optimizer Stack | PR | Content | |---|---| | linkedin#527 **(this)** | API and internal models | | linkedin#530 | Database Repos | | linkedin#531 | REST service | | linkedin#533 | Analyzer app | | linkedin#534 | Scheduler app | | #tbd | Spark BatchedOFD app | | #tbd | Infra, docker-compose, smoke test | ## Summary PR 0 of N in the optimizer stack. [Overall Project](https://docs.google.com/document/d/1oGQWkmlVw0HG-D4Nx37q0oUEQd53Ni4q5Q7h1voaZIQ/edit?tab=t.0#heading=h.vtl818e4m9f7) [Service Design doc](https://docs.google.com/document/d/1xYOD7iuPjZO05UWfT1Dkmf4lYNw4iOjqY10UT2X1FAg/edit?tab=t.0). Introduces the optimizer service API and internal model <img width="631" height="410" alt="image" src="https://github.com/user-attachments/assets/8833471d-069a-49a2-9a10-5eb7f3b96a72" /> ## Changes - [ ] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests ## 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. - [ ] Some other form of testing like staging or soak time in production. Please explain. This PR contains only the data model (entities, DTOs, converters). Repository tests follow in PR 1. Verified: - `./gradlew :services:optimizer:compileJava` passes - `./gradlew compileJava` (full project) passes with no regressions - Spotless formatting passes # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Summary This empty commit is in order to trigger the ELR process. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [ ] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request. Co-authored-by: Levi Jiang <lejiang@lejiang-mn2962.linkedin.biz> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
linkedin#594) ## Summary This is the follow up PR of linkedin#542. After the fix, we were able to replace the table once but not multiple times. RC anslysis: 1. `OpenHouseInternalTableOperations` uses `metadata.location()` for the new HTS tableLocation, and that value comes from `.withLocation(tableLocation)` that we pass, so we need to make sure it's schemeless. 2. We used `tableDto.getTableVersion()` to populate `.withLocation(tableLocation)`, but tableDto always contains scheme. (Why is tableLocation = tableVersion in tableDto? Because tableVersion comes from client request, and client request uses the tableLocation from the server response, so they will always be same and contain scheme) 3. The last PR fixes the tableLocation in table properties, so the new tableVersion for the first replace will be schemeless. But since the new tableLocation has scheme, the next replace will fail. Therefore, in this PR, we use the tableLocation from the table properties that we had just stripped in the last PR for `.withLocation(tableLocation)`. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [x] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [x] Manually Tested on local docker setup. Please include commands ran, and their output. - [ ] Added new tests for the changes made. - [x] 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. **Unit tests** I removed `stripPathScheme` in the spark E2E tests to gurantee that scheme issues can be captured by unit tests. **Test 1: RTAS** ``` scala> spark.sql(s"CREATE TABLE $tableName TBLPROPERTIES ('prop1'='val1', 'prop2'='val2') AS SELECT * FROM $sourceName"); res3: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"INSERT INTO $tableName values (4, 'd')"); res4: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"REPLACE TABLE $tableName PARTITIONED BY (part) TBLPROPERTIES ('prop1'='newval1', 'prop3'='val3') AS SELECT id, data, CASE WHEN (id % 2) = 0 THEN 'even' ELSE 'odd' END AS part FROM $sourceName ORDER BY 3, 1"); res5: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"REPLACE TABLE $tableName PARTITIONED BY (part) AS SELECT 2 * id as id, data, CASE WHEN ((2 * id) % 2) = 0 THEN 'even' ELSE 'odd' END AS part FROM $sourceName ORDER BY 3, 1"); res12: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"SELECT * FROM $tableName").show(false) +---+----+----+ |id |data|part| +---+----+----+ |2 |a |even| |4 |b |even| |6 |c |even| +---+----+----+ ``` **Test 2: CRTAS** ``` scala> spark.sql(s"CREATE OR REPLACE TABLE $tableName TBLPROPERTIES ('prop1'='val1', 'prop2'='val2') AS SELECT * FROM $sourceName"); res19: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"INSERT INTO $tableName values (4, 'd')"); res20: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"CREATE OR REPLACE TABLE $tableName PARTITIONED BY (part) AS SELECT id, data, CASE WHEN id % 2 = 0 THEN 'even' ELSE 'odd' END AS part FROM $sourceName ORDER BY 3, 1"); res21: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"CREATE OR REPLACE TABLE $tableName PARTITIONED BY (part) AS SELECT 2 * id as id, data, CASE WHEN ((2 * id) % 2) = 0 THEN 'even' ELSE 'odd' END AS part FROM $sourceName ORDER BY 3, 1"); res22: org.apache.spark.sql.DataFrame = [] scala> spark.sql(s"SELECT * FROM $tableName").show(false) +---+----+----+ |id |data|part| +---+----+----+ |2 |a |even| |4 |b |even| |6 |c |even| +---+----+----+ ``` # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request. --------- Co-authored-by: Levi Jiang <lejiang@lejiang-mn2962.linkedin.biz>
## Optimizer Stack | PR | Content | |---|---| | linkedin#527 | Data Model | | linkedin#530 **(this)** | Database Repos | | linkedin#531 | REST service | | linkedin#533 | Analyzer app | | linkedin#534 | Scheduler app | | #tbd | Spark BatchedOFD app | | #tbd | Infra, docker-compose, smoke test | ## Summary PR 1 of N in the optimizer stack. [Overall Project](https://docs.google.com/document/d/1oGQWkmlVw0HG-D4Nx37q0oUEQd53Ni4q5Q7h1voaZIQ/edit?tab=t.0#heading=h.vtl818e4m9f7) [Service Design doc](https://docs.google.com/document/d/1xYOD7iuPjZO05UWfT1Dkmf4lYNw4iOjqY10UT2X1FAg/edit?tab=t.0). Spring Data JPA repositories for all four optimizer tables with filtered query support, plus tests exercising save/find, filtered queries, upsert semantics, and append-only history. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests **Repositories**: `TableOperationsRepository`, `TableOperationsHistoryRepository`, `TableStatsRepository`, `TableStatsHistoryRepository` — each with JPQL filtered query methods. **Tests**: Repository tests for all four tables plus `OptimizerServiceContextTest` verifying the Spring context loads. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. `./gradlew :services:optimizer:test` — all tests pass (H2 in MySQL mode). # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…n#531) ## Optimizer Stack | PR | Content | |---|---| | linkedin#527 | Data Model | | linkedin#530 | Database Repos | | linkedin#531 **(this)** | REST service | | linkedin#533 | Analyzer app | | linkedin#534 | Scheduler app | | #tbd | Spark BatchedOFD app | | #tbd | Infra, docker-compose, smoke test | ## Summary PR 2 of N in the optimizer stack. Service layer and REST controllers for the optimizer service, plus the `apps/optimizer` shared module providing lightweight entity/repo copies for the analyzer and scheduler apps. ## Changes - [ ] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests **Service layer**: `OptimizerDataService` interface and `OptimizerDataServiceImpl` — CRUD operations, complete-operation lifecycle, stats upsert with history double-write, filtered queries. **Controllers**: `TableOperationsController`, `TableOperationsHistoryController`, `TableStatsController` — REST endpoints per the design doc API spec. **Shared module** (`apps/optimizer`): Lightweight entity and repository copies used by the analyzer and scheduler apps to read optimizer state directly from MySQL. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. H2 integration tests in `OptimizerDataServiceImplTest` (5 tests): - `completeOperation_writesHistoryFromOperationRow` — saves SCHEDULED row, completes it, asserts history DTO fields - `completeOperation_notFound_returnsEmpty` — completes nonexistent ID, asserts empty - `upsertTableStats_createsNewRow` — upserts new table, asserts DTO and repo row - `upsertTableStats_updatesExistingRow` — upserts twice, asserts overwrite with single row - `upsertTableStats_appendsHistoryOnEveryCall` — upserts twice, asserts 2 history rows ``` ./gradlew :services:optimizer:test # BUILD SUCCESSFUL — all 25 tests pass (repo tests from PR 1 + 5 new service tests) ``` # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Summary Dropping a table currently goes through `catalog.loadTable` twice: 1. `TablesServiceImpl.deleteTable` → `findById` parses metadata.json to build a `TableDto` for the ACL check. 2. `OpenHouseInternalCatalog.dropTable` → `loadTable(id).location()` parses metadata.json again to get the table base directory for `fileIO.deletePrefix` on purge. Both parses are redundant — everything drop needs (`databaseId`, `tableId`, `tableUUID`, the metadata.json path) is already on the HTS row. The redundant parses also make drop fragile: if metadata is semantically invalid (e.g. `Cannot find schema with current-schema-id=6 from schemas`), drop fails even though the HTS row and storage files are still well-formed. This PR removes both `loadTable` calls from the drop path: - **`OpenHouseInternalCatalog#findHouseTable(TableIdentifier)`** — HTS-only lookup that returns the `HouseTable` row, no metadata parse. - **`OpenHouseInternalRepository#findStubById(TableDtoPrimaryKey)`** — returns a partial `TableDto` (`databaseId`, `tableId`, `tableUUID`, `tableLocation`) built from the HTS row. Enough for the OPA auth check, which only uses db + UUID. - **`TablesServiceImpl#deleteTable`** uses `findStubById` instead of `findById`. - **`OpenHouseInternalCatalog#dropTable`** uses `findHouseTable` and derives the storage base location as the parent directory of the metadata.json path — same derivation `OpenHouseInternalRepositoryImpl#save` already uses in its replace flow, since OpenHouse writes `metadata.json` directly under `<base>/` (no `/metadata/` subdir). A side benefit: corrupted-metadata tables can now be dropped end-to-end through the normal API. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [X] Refactoring - [ ] Documentation - [ ] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [X] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request. --------- Co-authored-by: Dushyant Kumar <dukumar@linkedin.biz> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…zer (linkedin#569 follow-up) (linkedin#583) ## Summary PR linkedin#569 added datetime/date/time support to `filters._literal_to_sql()` so the Python → DataFusion SQL forward direction emits e.g. `CAST('2026-05-02 00:00:00.000000+0000' AS TIMESTAMP)`. However the reverse direction in `scan_optimizer._convert_comparison()` only handles bare `exp.Literal` nodes. When sqlglot parses the SQL back, the value is an `exp.Cast(exp.Literal, DataType(TIMESTAMP))` — so `isinstance(right, exp.Literal)` returns `False` and the comparison gets **silently dropped** from the pushdown `row_filter`. The result, for Iceberg tables partitioned by `day(timestamp_col)`, is that the datetime filter never reaches PyIceberg's manifest pruning. Every file in the snapshot gets scanned regardless of the predicate. Observed in production via a `dalids://` table: requesting `datepartition >= '2026-05-02' AND datepartition < '2026-05-04'` ended up reading whichever partitions happened to be at the head of the snapshot (`datepartition_day=2026-05-08` in our case). ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [x] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests This patch extends `_convert_comparison` + `_literal_to_python` in `scan_optimizer.py` to also accept `Cast(Literal, TIMESTAMP/DATE/TIME)` and convert it back to the matching Python `datetime`/`date`/`time` value, restoring partition pushdown. ## Testing Done - [x] Added new tests for the changes made. Added `test_cast_timestamp_pushed_as_datetime` covering `TIMESTAMP` (with tz), `TIMESTAMP` (without tz), and `DATE`. All three round-trip to the expected datetime/date `Filter`. Manually verified end-to-end: with the patch, an Iceberg scan with a daily-partitioned table only opens manifests whose partition value matches the datetime filter (instead of every manifest in the snapshot). ## Notes for Reviewers Direct follow-up to linkedin#569 — they go together. Without this fix, that PR's forward-direction support produces a SQL string that the scan optimizer can't reverse, so partition pushdown silently regresses for datetime predicates. The change is small and isolated to `scan_optimizer.py`.
## Optimizer Stack | PR | Content | |---|---| | linkedin#527 | Data Model | | linkedin#530 | Database Repos | | linkedin#531 | REST service | | linkedin#533 **(this)** | Analyzer app | | linkedin#534 | Scheduler app | | linkedin#599 | Spark BatchedOFD app | | #tbd | Infra, docker-compose, smoke test | ## Summary PR 3 of N in the optimizer stack. Introduces `apps/optimizer-analyzer`, a Spring Boot CommandLineRunner that evaluates every table in `table_stats` against pluggable `OperationAnalyzer` strategies. The first strategy, `OrphanFilesDeletionAnalyzer`, schedules OFD operations with 24h success / 1h failure retry cadence, a 6h SCHEDULED timeout, and a 5-strike circuit breaker. Key design choices: - Bulk-loads operations and history into maps (one query per type), then iterates the stats list — O(types) queries, not O(tables). - Uses the existing generic `find()` repository methods with null params. - Pure unit tests with Mockito — no Spring context needed. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests **Core**: `AnalyzerRunner` — loads table_stats, pre-loads operations and history into maps, evaluates each table against all analyzers, circuit breaker logic. **Strategy interface**: `OperationAnalyzer` — `isEnabled(table)`, `shouldSchedule(table, currentOp, latestHistory)`, `getCircuitBreakerThreshold()`. **Cadence policy**: `CadencePolicy` — encapsulates time-based retry logic shared across operation types. **OFD analyzer**: `OrphanFilesDeletionAnalyzer` — enabled via `maintenance.optimizer.ofd.enabled` table property. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. 25 unit tests: - `AnalyzerRunnerTest` (7 tests) — eligible table insertion, cadence skip, disabled table, shouldSchedule=false, null UUID, circuit breaker trip, below-threshold pass - `OrphanFilesDeletionAnalyzerTest` (18 tests) — isEnabled variants, shouldSchedule for no-op/PENDING/SCHEDULING/SCHEDULED with history combinations ``` ./gradlew :apps:optimizer-analyzer:test # BUILD SUCCESSFUL — 25 tests pass ``` # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Abhishek Nath <anath1@linkedin.com>
## Summary Reverts commit `637896219c53cad672a4a3d381c1dad302971854` — "Support returning HTS fields in table list api (linkedin#579)". The revert conflicted with linkedin#589 (merged after linkedin#579) in `OpenHouseInternalCatalog.java`. To make the conflict visible to the reviewer, this PR is split into two commits: 1. **Revert with conflict markers preserved** (`a15b0da5`) — raw `git revert` output, `OpenHouseInternalCatalog.java` left with `<<<<<<<` / `=======` / `>>>>>>>` markers. Committed with `--no-verify` because the markers fail spotless. 2. **Resolve conflict** (`387dd773`) — keep `findHouseTable` (from linkedin#589, still used by the drop path), drop `listHouseTables` (from linkedin#579, the new addition being reverted). The intent of the split is to let the reviewer inspect what conflicted, then see exactly how it was resolved. ## Changes - [X] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [X] Refactoring - [ ] Documentation - [X] Tests Reverts the public list-table API additions and the supporting tests from linkedin#579. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [ ] Added new tests for the changes made. - [X] 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. Tests added by linkedin#579 are removed as part of the revert. Relying on CI to confirm the remaining suite (including the linkedin#589 drop-path tests that depend on `findHouseTable`) still passes. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Summary Trigger build ## Testing Done build - [ ] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request.
…edin#619) Reverts the 17 commits that landed on main after v0.5.417, bringing the tree back to exactly the v0.5.417 state. Squashed into a single revert commit for reviewability and to allow reinstating everything as one unit (revert this commit to bring all 17 changes back). Reverted commits (v0.5.417..main, newest first): - Revert linkedin#579 (HTS fields in table list api) (linkedin#610) - feat(optimizer): [3/N] Analyzer (linkedin#533) - [DataLoader] Handle Cast(Literal, TIMESTAMP/DATE/TIME) in scan optimizer (linkedin#569 follow-up) (linkedin#583) - Skip metadata.json parse in drop path (linkedin#589) - feat(optimizer): [2/N] Optimizer REST Service and Controller (linkedin#531) - [BDP-102028] feat(optimizer): [1/N] Optimizer Database (linkedin#530) - [RTAS]: Fix bug - remove fs scheme from tableLocation in commit (cont) (linkedin#594) - Trigger ELR process (linkedin#593) - [BDP-102028] feat(optimizer): [0/N] Optimizer API and internal model (linkedin#527) - Fail retention app when the columnPattern mismatch partition spec (linkedin#552) - [DataLoader] Drop OpenTelemetry minimum version to 1.38.0 (linkedin#590) - [DataLoader] Emit OpenTelemetry metrics for read operations (linkedin#582) - Cache iceberg metadata to reduce redundant requests to storage (linkedin#509) - bump iceberg 1.2 version to 1.2.0.17 (linkedin#587) - Support returning HTS fields in table list api (linkedin#579) - [DataLoader] Add unique id property to OpenHouseDataLoader (linkedin#580) - [DataLoader] Add OpenTelemetry metrics support (linkedin#575) ## Summary <!--- HINT: Replace #nnn with corresponding Issue number, if you are fixing an existing issue --> [Issue](https://github.com/linkedin/openhouse/issues/#nnn)] Briefly discuss the summary of the changes made in this pull request in 2-3 lines. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [ ] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary
Adds a catalog-level CAS in `OpenHouseInternalTableOperations.doCommit`
that aborts when the writer's declared base (`COMMIT_KEY`) does not
match the catalog's current persisted base — closing the
`BaseTransaction.applyUpdates` silent-rebase variant of the stale-base
lost-update bug (incident-12185).
## Mechanism
1. A writer's transaction stages `COMMIT_KEY = T_X` and a
`SNAPSHOTS_JSON_KEY` payload computed against `T_X`.
2. A racing commit advances the catalog `T_X → T_Y`, adding a snapshot.
3. `BaseTransaction.applyUpdates` silently refreshes the in-flight base
to `T_Y` and re-applies the staged update — re-stamping `COMMIT_KEY =
T_X` on top of `T_Y` while leaving the stale `SNAPSHOTS_JSON_KEY`.
4. `doCommit` runs with `base = T_Y` but `COMMIT_KEY = T_X`. Without the
check, the subtractive snapshot merge computes `toRemove =
T_Y.snapshots() − stale payload = {racing snapshot}` and silently drops
it.
The fix reads `COMMIT_KEY` before `failIfRetryUpdate` strips it,
URI-normalizes both paths via Hadoop `Path`, and throws
`CommitFailedException` on mismatch so Iceberg retries against the fresh
base.
## Scope of the check
- **Aborts** when `COMMIT_KEY` is a concrete location differing from the
catalog base, or is `INITIAL_VERSION`.
- **Does not defend** commits that leave `COMMIT_KEY` unset — wholesale
replace/create (`replaceTable`, stage-create, stage-replace) are
authoritative over the snapshot set, so there is no stale base to
compare against.
## Changes
Bug fix + unit test, internal catalog only (2 files). `doCommit` may now
throw `CommitFailedException` on a stale-base commit that previously
silently dropped a racing snapshot.
## Testing
Unit test `testDoCommitMustAbortStaleBaseRebaseToPreventSnapshotLoss` in
`OpenHouseInternalTableOperationsTest` round-trips the post-refresh
`TableMetadata` through `TableMetadataParser` so
`base.metadataFileLocation()` is non-null (matching a loaded-from-disk
base) and the URI-normalized comparison runs. Asserts
`CommitFailedException` is thrown and `houseTableRepository.save` is
never invoked.
```
./gradlew :iceberg:openhouse:internalcatalog:test --tests "*OpenHouseInternalTableOperationsTest"
```
### Not covered
A Spark concurrent-insert behavior test
(`SparkConcurrentInsertFunctionalTest`, PR linkedin#614) was explored and
removed: it only reproduces against the H2 test fixture, not production
MySQL+HTS. A prod-realistic black-box repro would need the real HTS app
or a deployed instance.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Introduce an `all-modules` aggregate module that transitively depends on every published OpenHouse module. **Problem:** ELR (External Library Registry) at LinkedIn requires onboarding and monitoring each artifact individually. With 24+ OpenHouse modules per release, all 24 ELR checks must pass before a dependency bump can proceed. This creates operational overhead and makes non-happy-path failures harder to isolate and track. **Solution:** `all-modules` provides a single artifact that covers the full OpenHouse module set via transitive dependency resolution. **How it is used in practice:** In our downstream LinkedIn dependency (`li-openhouse`), `all-modules` is declared in the `external` block of `product-spec.json` — the registry ELR scans to enumerate tracked artifacts. ELR resolves `all-modules` transitively and sees all OpenHouse modules through that single entry. The individual 24 ELR registrations (e.g. `openhouse_tables-test-fixtures`, `openhouse_storage`, etc.) can then be decommissioned. During build time, `all-modules` is **not** placed on any classpath. Submodules continue to resolve their specific OpenHouse dependencies directly by GAV coordinate. The separation is intentional: `all-modules` is a tracking artifact for ELR, not a runtime dependency. ## Changes - [x] New Features `all-modules/build.gradle` — new aggregate module that dynamically depends on all leaf subprojects, following the same pattern used by the [Venice](https://github.com/linkedin/venice) project. `settings.gradle` — registers `:all-modules` as a Gradle subproject. ## Testing Done - [x] No tests added or updated. Please explain why. If unsure, please feel free to ask for help. This change adds a Gradle configuration module only. There is no production logic to test. The module is verified by the existing Gradle build resolving all subproject dependencies correctly at configuration time. # Additional Information - No breaking changes, deprecations, or large PR splits required.
…e) (linkedin#619)" (linkedin#625) This reverts commit d4fc9fe (linkedin#619), which had reverted all commits after v0.5.417. Restores the tree state to include the commits that were rolled back, including the optimizer service/analyzer modules, internal catalog cache, and dataloader changes. Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com>
## Optimizer Stack | PR | Content | |---|---| | linkedin#527 | Data Model | | linkedin#530 | Database Repos | | linkedin#531 | REST service | | linkedin#533 | Analyzer app | | linkedin#534 **(this)** | Scheduler app | | #tbd | Spark BatchedOFD app | | #tbd | Infra, docker-compose, smoke test | ## Summary PR 4 of N in the optimizer stack. Introduces `apps/optimizer-scheduler`, a Spring Boot CommandLineRunner that claims PENDING operations and submits batched Spark jobs via the Jobs Service. <img width="381" height="293" alt="image" src="https://github.com/user-attachments/assets/d8017b54-c6d5-4a7d-ad12-dc34c67e1cdb" /> State machine: Analyzer creates all Operations as PENDING 1. Scheduler marks PENDING as SCHEDULING to reserve the operation for a Bin ahead of scheduling to reduce duplicate job submission. 2. After claiming, the bin is submitted as a single job. If the batch is successfully submitted, the existing operations are updated to a status of scheduled with the associated jobID persisted. 3. If the submission fails, the job will be reset to pending so another scheduler run can pick it up. 4. Duplicate pending jobs are cancelled. 5. SCHEDULING is not expected to be in that state for long, minutes at most, and any operations that are stuck in the SCHEDULING status for >T period of time should move to a state of CANCELLED so next analyzer iteration will create a new PENDING op. This might happen if the scheduler was to suddenly crash or pod rotation. It may or may not happened after the job is scheduled. Therefore its safest to transition to CANCELLED in case the job was successful but not transitioned to scheduled. This is as-compared to failing to submit a job, in which case we know the job will never complete. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests **Scheduler runner**: Loads PENDING ops, bin-packs by file count, claims via two-step CAS (PENDING → SCHEDULING → SCHEDULED), submits one Spark job per bin. **Bin packer**: Greedy first-fit descending algorithm. Oversized tables get their own bin (never dropped). Tables with no stats default to cost 0. **Jobs client**: WebClient-based REST client submitting `POST /jobs` to the Jobs Service with table names, operation IDs, and results endpoint. **Repository additions**: Three `@Modifying` CAS methods on `TableOperationsRepository` — `cancelDuplicatePending`, `markScheduling`, `markScheduled` — required for safe concurrent scheduling. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. 13 unit tests: - `BinPackerTest` (7 tests) — empty input, single table, under/over limit, oversized table, no stats, descending sort - `SchedulerRunnerTest` (6 tests) — no pending ops, two-step claim + schedule, launch failure, already-claimed skip, duplicate cancellation, multi-row bin claim ``` ./gradlew :apps:optimizer-scheduler:test # BUILD SUCCESSFUL — 13 tests pass ``` # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Abhishek Nath <anath1@linkedin.com>
… POM resolves (linkedin#628) ## Summary The `:all-modules` aggregate added in linkedin#615 globs leaf subprojects: ```groovy rootProject.subprojects.each { subproject -> if (subproject.path != project.path && subproject.subprojects.isEmpty()) { implementation project(subproject.path) } } ``` The optimizer split (linkedin#533 introduced `:services:optimizer:analyzer` and `:apps:optimizer:analyzerapp`) turned `:services:optimizer` from a leaf into a parent, so the glob now picks up `analyzer` / `analyzerapp` instead — but neither module applied `openhouse.maven-publish`. Consumers resolving `all-modules:0.5.439` fail on `analyzerapp:0.5.439` / `analyzer:0.5.439` (and `analyzer`'s POM transitively pulls `optimizer:0.5.439`, also unpublished). JFrog falls through to the upstream public mirrors which return 401/403. Apply `id 'openhouse.maven-publish'` to `:services:optimizer`, `:services:optimizer:analyzer`, and `:apps:optimizer:analyzerapp` so their JARs/POMs land in JFrog alongside the aggregate. ## Changes - [x] Bug Fixes Apply `id 'openhouse.maven-publish'` to three previously-unpublished optimizer leaf modules so the `all-modules:0.5.x` aggregate POM resolves cleanly for downstream consumers. ## Testing Done - [x] Manually Tested on local docker setup. Please include commands ran, and their output. ``` ./gradlew :services:optimizer:publishToMavenLocal \ :services:optimizer:analyzer:publishToMavenLocal \ :apps:optimizer:analyzerapp:publishToMavenLocal \ :all-modules:publishToMavenLocal ``` All four tasks succeed. `~/.m2/repository/com/linkedin/openhouse/{optimizer,analyzer,analyzerapp,all-modules}/unspecified/` now contains main jar, lib jar (where applicable), sources, javadoc, and POM. Cross-module POM refs verified: - `analyzer.pom` → `com.linkedin.openhouse:optimizer` (compile) - `analyzerapp.pom` → `com.linkedin.openhouse:analyzer` (runtime) - `all-modules.pom` → `com.linkedin.openhouse:analyzer` and `com.linkedin.openhouse:analyzerapp` (runtime) `bootJar SKIPPED` on `:services:optimizer:analyzer` (library; `enabled = false`) does not break the publish — Gradle tolerates the skipped artifact task and `jar` (`archiveClassifier = ''`) is what ships. - [x] No tests added or updated. Build-system metadata change only; no source/runtime behavior changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## Summary This is a follow up of Optimizer series PRs (Previous PR linkedin#534). Introduces `BatchedOrphanFilesDeletionSparkApp`, the multi-table counterpart of the existing single-table `OrphanFilesDeletionSparkApp`. One Spark job now processes a list of `(table, operationId)` pairs that the optimizer scheduler bin-packed into a single batch, reporting SUCCESS/FAILED per operation back to the Optimizer Service. Stands up the **`optimizerclient` codegen module** so consumers (this app, and future ones) talk to the Optimizer Service via an auto-generated client rather than hand-rolled HTTP — mirrors the existing `:client:jobsclient`/`:client:tableclient` pattern. Lands a **first-fit-decreasing bin packer** that is used in the followup PR (linkedin#604 ) to assemble tables in batches. Also consolidated existing bin packer under this. ### Key design choices - **Per-table failure isolation** — exceptions in one table are caught, FAILED is posted for that operationId, and remaining tables continue. The job exits 0 if at least one table succeeds. - **Recoverable result reporting** — if `POST /v1/optimizer/operations/{id}/update` fails after retries, the row stays `SCHEDULED` and the Analyzer's stale-timeout will re-queue it. No retry storms in the Spark driver. - **Optional optimizer-service callbacks** — `--resultsEndpoint`/`--operationIds`/`--tableUuids` are all optional, so the legacy `JobsScheduler` can launch the app without optimizer integration. When absent, the per-operation callback is skipped; HTS `StateManager` still tracks per-job lifecycle. - **Scheduler decides parallelism, not the app** — `--driverParallelism` is honoured verbatim; the app never picks its own thread count. - **`MAX_BATCH_SIZE = 200`** — hard ceiling enforced at `buildEntries` (Spark side) so a misconfigured scheduler can't blow past `ARG_MAX`. Operating point stays `--batchMaxItems 25`. ### Optimizer client (codegen) - **`services:optimizer`** wired with the same `service-specgen-convention` + springdoc plugins as `services:jobs`/`services:tables`/`services:housetables`. Port 8003 (slots into the existing 8000/8001/8002 allocation). - **`client:optimizerclient`** — three-line `build.gradle` mirroring `client:jobsclient`; generates `com.linkedin.openhouse.optimizer.client.{api,model,invoker}` from the spec. - **`client:secureclient`** picks up `OptimizerApiClientFactory` (parallels `JobsApiClientFactory`) for SSL/auth-aware `ApiClient` construction. - **`OptimizerServiceClient`** in `apps:spark` is a thin wrapper around the generated `TableOperationsControllerApi`, mirroring `JobsClient`'s shape — `RetryTemplate` (new `RetryUtil.getOptimizerApiRetryTemplate()`), surface is `Optional<TableOperationsHistory> updateOperation(operationId, request)`. No more OkHttp, no more hand-rolled DTOs. ### Bin packer - `FirstFitDecreasingBinPacker` — FFD by weight with secondary caps on bytes and item count; oversized items get a dedicated bin. Reuses the existing `Bin`/`BinItem` types from linkedin#534. - FirstFitBinPacker.java and FirstFitBinPackerTest.java — deleted (renamed / consolidated). ### Batched Spark app - `BatchedOrphanFilesDeletionSparkApp` — extends `BaseSparkApp`; iterates entries via a fixed thread pool, reuses `Operations.deleteOrphanFiles(...)` per table, posts per-operation status, runs the existing `TableStateValidator` per table ### Additional changes **Service-side spec generation (`services/optimizer`)** - `build.gradle` — added `service-specgen-convention` + springdoc + processes plugins. `openApi.customBootRun` overrides the production MySQL DataSource with in-memory H2 at build time (overrides live in `build.gradle`, not a profile file, so they don't ship inside the fat jar). Port 8003. - `developmentOnly 'com.h2database:h2:2.1.210'` — H2 reaches `bootRun`/specgen but is excluded from `bootJar` and from `api`/`runtimeElements` propagation. Pinned to 2.1.210 to match the version `:iceberg:openhouse:htscatalog` already pulls in repo-wide and avoid the slf4j-2.x bump that 2.2.x would have introduced. **Client codegen (`client/optimizerclient`)** - New module (3-line `build.gradle`) generating `TableOperationsControllerApi`, `TableStatsControllerApi`, `TableOperationsHistoryControllerApi` + their DTOs. - `client/secureclient/OptimizerApiClientFactory.java` — SSL/auth-aware factory mirroring `JobsApiClientFactory`. - `settings.gradle` — registers `:client:optimizerclient`. **Bin packer (`libs/optimizer/binpack`)** - `FirstFitDecreasingBinPacker.java` (moved from apps/spark). Pre-projected `BinItem` inputs, weight + item-count caps, oversized items get their own bin. - `FirstFitDecreasingBinPackerTest.java` (moved + adapted to the libs `BinItem` interface). **Spark app (`apps/spark`)** - `spark/BatchedOrphanFilesDeletionSparkApp.java` — multi-table OFD with worker-thread pool, per-table failure isolation, `Iterables.size()`-based orphan count (bounded driver heap), `MAX_BATCH_SIZE=200` guard. - `spark/optimizer/OptimizerServiceClient.java` — wraps generated `TableOperationsControllerApi`. - `util/RetryUtil.java` — added `getOptimizerApiRetryTemplate()`. - `build.gradle` — depends on `:libs:optimizer:binpack` with a targeted `exclude` on `log4j-slf4j2-impl` (transitively brought by services:optimizer; conflicts with apps:spark's bundled `log4j-slf4j-impl` 1.x bridge). **Test using CLI:** --tableNames db.t1,db.t2,db.t3 --operationIds op-uuid-1,op-uuid-2,op-uuid-3 --tableUuids tab-uuid-1,tab-uuid-2,tab-uuid-3 --resultsEndpoint http://optimizer.svc:8080 --driverParallelism 4 plus existing OFD knobs (`--ttl`, `--backupDir`, `--concurrentDeletes`, `--streamResults`, `--maxOrphanFileSampleSize`). <!--- HINT: Replace #nnn with corresponding Issue number, if you are fixing an existing issue --> [Issue](https://github.com/linkedin/openhouse/issues/#nnn)] Briefly discuss the summary of the changes made in this pull request in 2-3 lines. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. **Open items for reviewers:** - `OptimizerServiceClient` is add on top of the generated `TableOperationsControllerApi` — matches `JobsClient`'s pattern. Fine, or should it live inside `:client:optimizerclient` as a higher-level facade? - Apps:spark currently has to `exclude log4j-slf4j2-impl` when depending on `:libs:optimizer:binpack` because services:optimizer transitively brings the slf4j-2.x bridge while apps:spark ships the 1.x bridge. A repo-wide bridge-alignment cleanup would let us drop this exclude; happy to file a separate issue. - The bin packer ends up as two siblings in libs (`FirstFitBinPacker` — optimizer-coupled; `FirstFitDecreasingBinPacker` — agnostic). Consolidation is done in FirstFitDecreasingBinPacker. - Builder defaults (maxWeightPerBin = 1_000_000L, maxItemsPerBin = 50) are sized for OFD. If we expect other operation types with materially different cost shapes, we may want to move the defaults out of the packer class and into the per-operation config (Spring @value already provides this in SchedulerConfig). For all the boxes checked, include additional details of the changes made in this pull request.
…fault (linkedin#624) ## Summary Stops a small read `batch_size` (e.g. 128) from lowering DataFusion's internal `datafusion.execution.batch_size` below its 8192 default. The read `batch_size` is now only ever *raised* into the execution batch size to honor a large requested batch (the case linkedin#568 fixed) — never lowered. Rationale: lowering the engine's internal execution batch below its default can only ever split work into more, smaller batches; it never coalesces rows up, so it cannot help and can only add per-batch overhead. We therefore only raise it for the large-batch case linkedin#568 added, and otherwise leave DataFusion at its default. > **Scope note:** this began as an investigation into a reported training perf regression (`read_batch` stalls, starved prefetch queue). Follow-up with Jonathan showed the regression **persisted even with the execution batch size pinned at 8192**, so that slowdown comes from elsewhere (the read/IO path), not this config. This PR lands as a small **correctness/hardening change**, not as the fix for that regression. ## Changes - [ ] Client-facing API Changes - [ ] Internal API Changes - [x] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests One change in `_create_transform_session`: only set `datafusion.execution.batch_size` when the read `batch_size` **exceeds** DataFusion's 8192 default; a smaller value leaves DataFusion at its default. No new public parameter (per review feedback to avoid extra config surface) — this is a clamp on the existing `batch_size`. ## 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. - [ ] Some other form of testing like staging or soak time in production. Please explain. The existing `test_split_batch_size_honored_with_transform` already covers the only behavior the dataloader exercises — a `batch_size` above the default is propagated so transforms are not fragmented — and still passes under the clamp. The added behavior is purely a guard against lowering the value below the default, which the dataloader's read path never benefits from. Full suite green: `make verify` — ruff + mypy + **281 passed**. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. Backward compatible. The only behavior change is that a read `batch_size` ≤ 8192 no longer shrinks the transform execution batch below DataFusion's default.
…#621) ## Summary <!--- HINT: Replace #nnn with corresponding Issue number, if you are fixing an existing issue --> Depends on linkedin#601 Allowlist entries are now Java regular expressions matched against the property key. ## Changes - [x] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done <!--- Check any relevant boxes with "x" --> - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. ./gradlew :services:tables:test --tests IcebergSnapshotsApiHandlerAuditTest # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request. Co-authored-by: James Wang <jamewang@linkedin.com>
…mit (linkedin#648)" (linkedin#665) # Problem & Solution Overview This reverts commit e80d45d (linkedin#648). linkedin#648 changed `OpenHouseInternalCatalog#listTables(Namespace)` to internally paginate HTS "list all" queries (page size 1000) instead of issuing a single unpaginated request, in order to avoid `DataBufferLimitException` on very large databases. In production, this caused `GET /v1/databases` (empty-namespace list-tables path) to fan out into many sequential HTS calls per request instead of one. For a deployment with a large number of databases/tables, this significantly increased request volume against HTS, saturating its JDBC connection pool (Hikari) and causing collateral failures on unrelated HTS calls (e.g. `HikariPool-1 - Connection is not available, request timed out after 30000ms`, `java.io.EOFException: connection was unexpectedly lost`, `PrematureCloseException: Connection prematurely closed BEFORE response`). This PR reverts the pagination change to restore the prior unpaginated `listTables` behavior while a safer fix (e.g. bounded concurrency, smarter page sizing, or pushing pagination to the client-facing paginated APIs only) is designed. # Testing Done - `git revert e80d45d` applied cleanly with no conflicts against current `main` (two unrelated commits, linkedin#650 and linkedin#651, merged on top since linkedin#648; neither touches this file). - `./gradlew :iceberg:openhouse:internalcatalog:compileJava` compiles cleanly after the revert. - Risk: low — this is a straight revert of a recent, isolated change back to previously-running production behavior (pre-linkedin#648). Follow-up work is needed to re-address the original `DataBufferLimitException` motivating linkedin#648, without triggering HTS connection-pool exhaustion.
## Summary Table creation filtered feature-gated preserved properties because `allowKeyInCreation` called the advised `isKeyPreservedForTable` method through Spring self-invocation, bypassing the feature-toggle aspect. Invoke the table-aware check from the repository's Spring proxy before applying the create-only allowlist fallback. This makes CREATE consistent with ALTER while retaining support for extension-defined properties that are writable only during creation. ## Changes - [x] Bug Fixes - [x] Tests - Evaluate `isKeyPreservedForTable` through the proxied `PreservedKeyChecker` during creation. - Retain `allowKeyInCreation` as the fallback for create-only preserved properties. - Add a Spring integration regression test using a real active table feature toggle. - Update the default-file-format test to model preserved/toggled state on the table-aware method. ## Testing Done - [x] Added new tests for the changes made. - [x] Updated existing tests to reflect the changes made. - [x] Some other form of testing like staging or soak time in production. Please explain. - Verified the new regression test fails without the production change and passes with it. - `:services:tables:test` — 463 tests passed. - `:services:tables:spotlessCheck` - `:services:tables:checkstyleMain` - `:services:tables:checkstyleTest` - Manually reproduced the original behavior against a live OpenHouse namespace: feature-gated properties survived ALTER but were silently filtered during CREATE. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description.
…#635) ## Summary This adds **Renovate** — an open-source tool that watches dependency versions and opens a pull request when a newer one is available — and points it at the LinkedIn build of Iceberg (`com.linkedin.iceberg`, the table library OpenHouse depends on). A scheduled job runs it every hour on GitHub's own machines. OpenHouse uses two versions of this library at the same time: one in the `1.2.x` series (any version starting with `1.2.`) and one in the `1.5.x` series. The tool keeps each series on its own newest version and never lets one cross into the other. First-run result, confirmed locally without opening any pull requests: - `iceberg-core`: `1.2.0.17` → `1.2.0.18` - `iceberg-core`: `1.5.2.11` → `1.5.2.15` ## Changes - [x] New Features - [x] Documentation - `.github/renovate.json` — the rules above. - `.github/workflows/renovate.yml` — the hourly scheduled job. - `docs/development/renovate-iceberg-sync.md` — setup and reasoning. ## Testing Done - [x] No tests added or updated. Ran Renovate locally in a mode that reads the repo and looks up versions but opens no pull requests; it proposed exactly the two updates above, each staying inside its own series. Also checked `renovate.json` with Renovate's official config validator. ## Additional Information - [ ] Breaking Changes - [ ] Deprecations **Optional:** adding a repository secret named `RENOVATE_TOKEN` lets the update pull requests run their tests automatically. GitHub will not start test runs on a pull request opened with a workflow's built-in credentials (a guard against workflows triggering each other), so without this secret the pull requests still open but you start their tests by hand. A personal access token or a GitHub App token works. It is not required to merge this change. This covers the two main version numbers only. The separately pinned `iceberg-aws` (`1.2.0.6`) is left out on purpose and can be added the same way later.
## Summary Problem: I have a feature which I want to ramp on the server. but I also don't want to prevent table owners from self-serve opting in. A generic function to handle that overlap doesn't exist today. Extend the existing `TableFeatureToggle` with self-service table overrides while preserving its server-managed targeting API. An explicit `<featureId>.enabled=true|false` table property wins. When the property is absent, activation delegates to the server toggle. Server rules now support trailing-`*` prefix matching independently for database and table names. ## Changes - [ ] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests Adds a binary-compatible default method to `TableFeatureToggle`: ```java isFeatureActivatedWithOverride(TableDto tableDto, String featureId) ``` It is deliberately **not** an overload of `isFeatureActivated`. The two carry different safety contracts, and a distinct name makes the difference visible at the call site: authorization gates such as `enable_mor` decide whether a user may write a preserved table property, so they must keep using the server-only `isFeatureActivated(String, String, String)`. The override-honoring form reads a property the gated user can write. An override that is neither `true` nor `false` fails closed: it is logged and the feature is treated as inactive. The gate is evaluated on the table-load path, so throwing would turn a typo like `read-bridge.enabled=flase` into a `400` and make the table unloadable. Extends the existing toggle rule matcher while preserving exact and `*` matching: - `tracking.events` matches exactly. - `tracking_*.events_*` matches database and table prefixes. - `*.*` matches every table. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] 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. Ran: ```shell JAVA_HOME=$(/usr/libexec/java_home -v 17) ./gradlew \ :services:tables:test \ --tests 'com.linkedin.openhouse.tables.toggle.TableFeatureToggleTest' \ :services:housetables:test \ --tests 'com.linkedin.openhouse.housetables.mock.WildcardTableToggleRuleMatcherTest' \ -x CopyGitHooksTask ``` All 12 focused tests passed, covering server fallback, explicit opt-in and opt-out, fail-closed handling of unparseable overrides, exact matching, wildcard matching, and paired database/table prefix matching. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. This is an independent OSS foundation for the read-bridge stack in linkedin#645 and the corresponding `li-openhouse` implementation PRs. Feature-specific default derivation remains outside this PR. Note for reviewers: the matcher change widens any existing `table_toggle_rule` row whose pattern ends in `*` but is not exactly `*`. Those previously matched nothing. Worth auditing HTS before merge.
## Summary **Problem:** OpenHouse DataLoader errors could not be reliably correlated with the corresponding Tables Service request. Timestamp-based correlation is noisy, and authentication failures were represented as generic `OSError`s and retried as if they were transient I/O failures. **Solution:** assign a unique `X-Request-ID` to every outbound catalog HTTP request and expose it through shared, typed catalog exceptions. The exception hierarchy centralizes request-ID formatting and distinguishes authentication, authorization, not-found, transport, HTTP, and malformed-response failures. ## Changes - [x] Client-facing API Changes - [x] Internal API Changes - [x] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [x] Refactoring - [ ] Documentation - [x] Tests Details: - Added a central request-aware HTTP session that generates a fresh UUID for every prepared request. - Added shared exception types: - `OpenHouseCatalogError` - `OpenHouseRequestError` - `OpenHouseTransportError` - `OpenHouseHTTPError` - `OpenHouseAuthenticationError` - `OpenHouseAuthorizationError` - `OpenHouseNoSuchTableError` - `OpenHouseInvalidResponseError` - Centralized `X-Request-ID` rendering and exposed it as `exception.request_id`. - Preserved compatibility with callers catching PyIceberg `NoSuchTableError`. - Classified 401/403 and other non-transient 4xx failures as non-retryable. - Kept transport failures, HTTP 408/429, and 5xx responses retryable. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] Added new tests for the changes made. - [x] 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. - [x] Some other form of testing like staging or soak time in production. Please explain. Validation performed: - `make verify` - Ruff lint passed - Ruff formatting passed - Mypy passed - **272 unit tests passed** - Added coverage for unique request IDs, typed 401/403/404/500 errors, transport errors, malformed JSON, missing/empty `tableLocation`, retryable failures, and non-retryable authentication failures. - Used a known request ID against the deployed OpenHouse route and confirmed it appeared in the Nginx access log; follow-up proxy configuration work is tracked separately to preserve the same value through Ambassador. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description.
) ## Summary Phase 1.5 only from `jobs-observability-plan.md` §10 — localized log lines, no new classes or entry-point changes: - **`OH_SCHED_START`** in `JobsScheduler.main`: echoes `numParallelJobs`, poller/submitter counts, poll/timeout settings (replaces brittle `values.yaml case()` for cap/deadline KQL). - **`OH_SCHED_ELIGIBLE`** in `OperationTasksBuilder`: stable eligible-count token alongside existing `metadata fetched count` log. - **Queued-timeout poll exit** in `OperationTask`: appends `lastObservedState` + `executionId` to distinguish genuine GGW queue timeout vs poll-lag give-up on terminal jobs. ~35 lines across 3 files. Phase 2 (OTEL gauges, heartbeat sampler, DLQ counters) intentionally deferred. ## Test plan - [x] Compiles; no test signature changes - [ ] Post OSS→LI bump: grep cron pod logs for `OH_SCHED_START`, `OH_SCHED_ELIGIBLE`, `lastObservedState=` on queued-timeout lines Co-authored-by: Cursor <cursoragent@cursor.com>
…kedin#667) This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [com.linkedin.iceberg:iceberg-core](https://redirect.github.com/linkedin/iceberg) | `1.2.0.19` → `1.2.0.20` |  |  | --- ### Release Notes <details> <summary>linkedin/iceberg (com.linkedin.iceberg:iceberg-core)</summary> ### [`v1.2.0.20`](https://redirect.github.com/linkedin/iceberg/releases/tag/v1.2.0.20) [Compare Source](https://redirect.github.com/linkedin/iceberg/compare/v1.2.0.19...v1.2.0.20) <sup><sup>*Changelog generated by [Shipkit Changelog Gradle Plugin](https://redirect.github.com/shipkit/shipkit-changelog)*</sup></sup> ##### 1.2.0.20 - 2026-07-31 - [0 commit(s)](https://redirect.github.com/linkedin/iceberg/compare/v1.2.0.20...v1.2.0.20) by - No notable improvements. No pull requests (issues) were referenced from commits. </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://redirect.github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4yODguMCIsInVwZGF0ZWRJblZlciI6IjQzLjI4OC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## Summary `CREATE OR REPLACE ... AS SELECT` (RTAS) silently dropped the table's policies. The replace path rebuilt the `policies` table property purely from the incoming request, so a replace that omitted policies wiped the existing retention, sharing, PII column tags, replication, and history, even though ordinary user table properties survived. ## Fix Policies are table metadata that a replace must not silently drop. Before the replace properties are built, the existing table's policies are merged with the request's policies. The merge is based on the existing policies, so any plane that the request does not explicitly provide is carried forward from the existing table, and each plane that the request does provide overrides the existing value. ## Merge behavior The table below describes how each policy plane behaves during a replace. | Plane | When the request provides it | When the request omits it | |---|---|---| | `retention` | The request value is applied. | The existing value is carried forward. | | `replication` | The request value is applied. | The existing value is carried forward. | | `history` | The request value is applied. | The existing value is carried forward. | | `lockState` | The request value is applied. | The existing value is carried forward. | | `columnTags` | The request map replaces the existing map in full. This is an overwrite, not a per-key merge. | The existing map is carried forward. An omitted field and an empty map are treated the same way. | | `sharingEnabled` | Not applicable, because this is a primitive boolean and a provided value cannot be distinguished from an omitted one. | The existing value is carried forward. | Two consequences follow from this behavior. First, because column tags use overwrite semantics and an empty map is treated the same as an omitted field, a replace cannot clear all column tags. Clearing tags is done with `ALTER TABLE ... MODIFY COLUMN ... UNSET TAG`. Second, because `sharingEnabled` is a primitive boolean with no unset state, its value is always preserved across a replace. Sharing is changed with `ALTER TABLE ... SET POLICY (SHARING=...)`. Spark RTAS has no policy clause, so it always sends a request with no policies, and the entire existing policies object is carried forward unchanged. A partial policy payload can only arrive from a client that calls the REST API directly. This behavior is consistent with the intent of RTAS, which should preserve table properties so that a replace does not require re-granting access to the same entity. ## Testing Done The REST level partial payload path is exercised through `RepositoryTest`, which is the layer that can send a partial `Policies` object. Spark cannot reach this path because it always sends a request with no policies. - `testReplaceMergesExistingPolicies` replaces a table without policies and asserts that the retention policy survives. - `testReplaceAppliesRequestedPolicies` asserts that a retention policy provided on the request is applied. - `testReplaceWithPartialPoliciesPreservesSharing` sends a partial payload containing only retention and asserts that `sharingEnabled` stays true while the new retention is applied. - `testReplaceWithPartialPoliciesPreservesOmittedPlanes` overrides only retention and asserts that the omitted history plane is carried forward. - `testReplaceWithPartialPoliciesPreservesColumnTags` sends a payload that provides retention but omits column tags, and asserts that the existing column tag is carried forward. - `testReplaceOverwritesColumnTags` sends a new column tag map and asserts that it replaces the existing map in full, dropping the previous tag. Black box coverage is exercised through `RtasPolicyPreservationTest` against an embedded OpenHouse server driven by Spark SQL. It asserts that retention, sharing, the PII column tag, and history all survive a `REPLACE TABLE ... AS SELECT`. The existing `SnapshotsControllerTest.testPutSnapshotsReplaceCommit` still passes, which confirms that a replace on a table that never had policies still yields none. `./gradlew :services:tables:test` and `:integrations:spark:spark-3.1:openhouse-spark-itest:catalogTest` pass on JDK 17, and Spotless is clean on the module. --------- Co-authored-by: mkuchenbecker <mkuchenbecker@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the open-source read-bridge feature on top of the per-table `config` channel (linkedin#644): - ColumnDefaultsSource: the single open-source/closed-source seam (field-id -> Iceberg single-value JSON). Open-source default is a no-op lambda in ApiConfig; a deployment overrides this bean (e.g. li-openhouse, from avro.schema.literal). - ReadBridgeConfigResolver: server-side encoder that stamps each default as a flat namespaced config entry (openhouse.read-bridge.column-default.<fieldId> = single-value JSON) — no envelope/POJO; the config map carries the structure. - ReadBridge (client): decodes those entries and applies the overlay at metadata-load time (the column-default transform is a marked TODO, and the place further V3 features get backported). Keeps loadMetadata to one call. Behaviorless until a ColumnDefaultsSource is supplied; fail-closed throughout. Co-authored-by: Cursor <cursoragent@cursor.com>
A known openhouse.read-bridge.column-default.* entry is produced by the server encoder from a typed JsonNode keyed by an integer field-id, so its suffix always parses as an int and its value always round-trips through readTree. A decode failure is therefore an encoder bug or transport corruption, not an expected input -- skipping it would silently read NULL instead of the column's default and hide a real defect. Throw instead. Unknown keys (a newer server feature this client doesn't recognize) are still ignored, preserving forward compatibility. Update javadoc/comments to the encoder round-trip rationale and note the guarantee covers well-formedness, not default-to-schema correctness (a write-time concern). Tests updated to assert fail-loud on bad field-id and unparseable value, plus forward-compat skip of unknown keys.
The fail-loud change updated ReadBridge but left two docs stating the old fail-closed behavior: - ColumnDefaultsSource told implementers they "must never throw", the opposite of the policy, and unimplementable alongside validating that a declared default binds to its column. Restate it as the capability-gap vs invariant-violation split: an empty map means nothing to bridge, while a declared-but-unhonorable default throws. - OpenHouseTableOperations.loadMetadata still claimed "unparseable config leaves the raw metadata untouched", which no longer holds: ReadBridge throws on a malformed known entry and loadMetadata is its only caller. Comment-only; no behavior change. Co-authored-by: Cursor <cursoragent@cursor.com>
loadMetadata is wrapped in Tasks.retry(20) for transient metadata *file* reads. Decode and apply are deterministic: a malformed config fails the same way every attempt, so retrying only burns ~90s and re-reads storage to reproduce an error already known on the first try. Keep from/apply inside loadMetadata (decode before IO so storage is never touched on a bad config), and wrap IllegalStateException as Tasks.UnrecoverableException — Iceberg already stops retry on that type. No doRefresh decode field or AtomicReference needed. Testing Done: - :integrations:java:iceberg-1.2:openhouse-java-itest:test --tests '*ReadBridge*' --tests '*OpenHouseTableOperationsTest.testMalformed*' --tests '*OpenHouseTableOperationsTest.testDoRefresh*'
Co-authored-by: Cursor <cursoragent@cursor.com>
Iceberg skips the loader when tableLocation is unchanged, so writing config on every GET desyncs stamps from in-memory overlays. Bind after apply; skip-reload leaves the pair intact. Co-authored-by: Cursor <cursoragent@cursor.com>
loadMetadata runs before Iceberg's UUID check. Setting config there desyncs stamps from current() if that check fails. Bind only after refreshFromMetadataLocation returns and the loader actually ran. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the deployment-specific ColumnDefaultsSource optional and data-only; OpenHouse owns the read-bridge.column-default feature id, self-serve enabled property, and fail-open toggle lookup before asking for defaults.
Correct the self-service property name and drop the contradictory "cluster kill switch" claim. Add coverage for a real source that returns no defaults, and assert table-property opt-in never calls the server toggle. Testing Done: - :services:tables:test --tests '*ReadBridgeConfigResolverTest'
BaseTableFeatureToggle looks up (databaseId, tableId, featureId) exactly; claiming a * / * fleet kill switch was inaccurate. Also note HTS is only hit when the self-service property is absent.
Replace essay javadoc with brief ownership, contract, ObjectProvider, and fail-open notes so the PR description carries the design narrative.
Overlays must not persist. Remove initial-default on field-ids the config stamped; unstamped ids keep writer defaults. Bridge XOR native: the encoder does not stamp ids that already have on-disk defaults.
An empty GET that Iceberg skips must not let overlays persist; strip uses currentConfig from the last load, not the latest GET. Co-authored-by: Cursor <cursoragent@cursor.com>
Overlay stamped column defaults onto schema field objects by field-id using the sanitize JSON helpers.
Unpublished OSS tables/fixtures need this so a SNAPSHOT uber can no-op drop/grant/lock on X-OH-Wap-Branch.
…branch. Stop rewriting schema, properties, and policies in the client so isolation is the X-OH-Wap-Branch skip, including partition-spec eligibility.
Jobs keep the catalog they already load. Tables REST body is unchanged; X-Iceberg-Ref aliases X-OH-Wap-Branch for a later Iceberg REST 1.11 ref field. DROP/RENAME/GRANT on a non-main WAP branch skip house-table identity mutations.
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
12 participants
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.
Summary
OpenHouseSparkCatalog(Spark 3.5 isolate + WAP enable; Spark 3.1 is the named SparkCatalog).GitForDataSparkCatalogis a deprecated subclass.X-OH-Wap-Branchand Iceberg REST 1.11-shapedX-Iceberg-Ref(same value). Iceberg REST 1.11 carriesref/ref-nameinside commit payloads; extra headers are ignored until OpenHouse speaks that body.write.wap.enabled; Iceberg creates a missing WAP ref on commit.Testing Done
WapBranchTestreadsX-Iceberg-Refwhen the OpenHouse header is absentOpenHouseSparkCatalogviaTestSparkSessionUtil