mengw15 commented on code in PR #7473:
URL: https://github.com/apache/texera/pull/7473#discussion_r3754836434
##########
common/config/src/main/scala/org/apache/texera/common/config/StorageConfig.scala:
##########
@@ -93,7 +95,9 @@ object StorageConfig {
// Per-user warehouses (#6870). On only when the switch is on AND the
catalog is REST
// (Lakekeeper): warehouses are Lakekeeper entities, so any other catalog
type keeps
// the feature off regardless of the switch.
- val warehouseEnabled: Boolean =
+ // warehouseEnabled is a var because tests need to exercise the enabled path
+ // (mirroring s3Endpoint above); production code never writes it.
+ var warehouseEnabled: Boolean =
Review Comment:
Fair enough — reverted to `val` in 365bd519f. The flag is a constructor
dependency of `WarehouseResource` now (alongside the client), so the spec
covers both gate states with two instances instead of mutating global config.
##########
amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala:
##########
@@ -535,6 +537,8 @@ class SyncExecutionResource extends LazyLogging {
storageUriOption match {
case Some(storageUri) =>
+ // Refuse to read a per-user-warehouse result while the feature is
off (#6930).
+ WarehouseReadGuard.assertReadable(storageUri)
Review Comment:
Refined in 365bd519f: rather than hoisting the lookups out of the try (which
would have made DB failures propagate too), the guard now throws a typed
`WarehouseUnavailableException` that both catch-alls rethrow — only the
kill-switch refusal escapes, every other failure keeps its existing handling.
##########
amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala:
##########
@@ -769,6 +773,8 @@ class SyncExecutionResource extends LazyLogging {
val uriOption = getConsoleMessageUri(executionId, OperatorIdentity(opId))
uriOption.flatMap { uri =>
+ // Refuse to read per-user-warehouse console messages while the
feature is off (#6930).
+ WarehouseReadGuard.assertReadable(uri)
Review Comment:
Same approach in 365bd519f — the catch-all rethrows
`WarehouseUnavailableException` and swallows the rest as before.
--
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]
