Copilot commented on code in PR #8713:
URL: https://github.com/apache/texera/pull/8713#discussion_r4114338697
##########
common/config/src/main/resources/storage.conf:
##########
@@ -152,10 +152,11 @@ storage {
interval-minutes = ${?STORAGE_CLEANUP_INTERVAL_MINUTES}
}
- # Per-user warehouse feature (#6870). One switch that hides the whole
feature until it
- # is fully built: merging the warehouse PRs changes nothing while this
stays off.
+ # Per-user warehouse feature (#6870). Only takes effect with the REST
catalog
+ # (Lakekeeper), since warehouses are Lakekeeper entities; any other
catalog type
+ # keeps the feature off whatever this says.
warehouse {
- enabled = false
+ enabled = true
Review Comment:
With the repository's default `rest` catalog, this makes
`StorageConfig.warehouseEnabled` true in ordinary local test runs.
`ExecutionResultServiceSpec.scala:953-963` and
`ResultExportServiceSpec.scala:1140-1153` still explicitly rely on the old
false default and expect `WarehouseUnavailableException`; the guard no longer
throws, so those non-integration specs continue into catalog access and fail.
CI masks this via its `postgres` override. Update those specs to use an
explicitly disabled flag (or otherwise isolate them from the deployment
default).
##########
common/config/src/test/scala/org/apache/texera/common/config/StorageConfigSpec.scala:
##########
@@ -50,12 +50,15 @@ class StorageConfigSpec extends AnyFlatSpec with Matchers {
StorageConfig.ENV_CLEANUP_INTERVAL_MINUTES shouldBe
"STORAGE_CLEANUP_INTERVAL_MINUTES"
}
- "StorageConfig warehouse settings" should "default the feature to disabled
so merging changes nothing" in {
- // storage.warehouse.enabled is the kill switch for the whole per-user
warehouse
- // feature (#6870); this guards the safe default from silently flipping to
true.
- // Only assert when the env override is unset (e.g. in CI), since it would
win otherwise.
+ "StorageConfig warehouse settings" should "be on by default, and only with
the REST catalog" in {
+ // storage.warehouse.enabled is the switch for the whole per-user warehouse
+ // feature (#6870), on since #8712. Warehouses are Lakekeeper entities, so
the
+ // switch is conjoined with the catalog type: any other catalog keeps the
+ // feature off however the switch reads, which is what stops a deployment
+ // without Lakekeeper from being handed a feature it cannot serve.
+ // Only assert when the env override is unset, since it would win
otherwise.
if (sys.env.get(StorageConfig.ENV_WAREHOUSE_ENABLED).isEmpty) {
- StorageConfig.warehouseEnabled shouldBe false
+ StorageConfig.warehouseEnabled shouldBe
(StorageConfig.icebergCatalogType == "rest")
Review Comment:
The amber/common CI job pins `STORAGE_ICEBERG_CATALOG_TYPE=postgres`
(`.github/workflows/build.yml:298-315`), so this expression evaluates to
`false` there whether the resource default is `true` or `false`. Consequently,
reverting the switch flip would still pass CI. Assert the raw resource default
as well so the behavior introduced by this PR is covered deterministically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
