kunwp1 commented on code in PR #7754:
URL: https://github.com/apache/texera/pull/7754#discussion_r3813998452
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -131,21 +134,35 @@ class WarehouseResource(client: LakekeeperClient,
enabled: Boolean) extends Lazy
throw new WebApplicationException(s"a warehouse named '$name' already
exists", 409)
}
- val warehouseName = s"user-$uid-$name"
+ // Never derive the catalog name from `name`: it is also the S3 key prefix
and a
+ // component of every result URI an execution wrote, so a name-derived one
could
+ // never change again. Drawing the id up front keeps the creation order
below
+ // intact. The sequence is looked up rather than named literally -- its
generated
+ // name is not a stable contract.
+ val whid: Integer = context.fetchValue(
+ DSL.field(
+ "nextval(pg_get_serial_sequence({0}, {1}))",
+ classOf[Integer],
+
DSL.inline(s"${USER_WAREHOUSE.getSchema.getName}.${USER_WAREHOUSE.getName}"),
+ DSL.inline(USER_WAREHOUSE.WHID.getName)
+ )
+ )
+ val lakekeeperWarehouseName = s"user-$uid-$whid"
Review Comment:
Can you add an object and define this variable there? I see so many
definition of this in the test spec file.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResourceSpec.scala:
##########
Review Comment:
I think you can refactor this line with line 230-242.
##########
common/dao/src/test/scala/org/apache/texera/dao/UserWarehouseSpec.scala:
##########
Review Comment:
rename this test title because we changed `warehouse_name` to
`lakekeeper_warehouse_name`.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -131,21 +134,35 @@ class WarehouseResource(client: LakekeeperClient,
enabled: Boolean) extends Lazy
throw new WebApplicationException(s"a warehouse named '$name' already
exists", 409)
}
- val warehouseName = s"user-$uid-$name"
+ // Never derive the catalog name from `name`: it is also the S3 key prefix
and a
+ // component of every result URI an execution wrote, so a name-derived one
could
+ // never change again. Drawing the id up front keeps the creation order
below
+ // intact. The sequence is looked up rather than named literally -- its
generated
+ // name is not a stable contract.
+ val whid: Integer = context.fetchValue(
+ DSL.field(
+ "nextval(pg_get_serial_sequence({0}, {1}))",
+ classOf[Integer],
+
DSL.inline(s"${USER_WAREHOUSE.getSchema.getName}.${USER_WAREHOUSE.getName}"),
+ DSL.inline(USER_WAREHOUSE.WHID.getName)
+ )
+ )
Review Comment:
Can't we just use UUID instead of asking the database to get a sequence
number?
##########
amber/src/main/scala/org/apache/texera/web/model/websocket/request/WorkflowExecuteRequest.scala:
##########
@@ -37,6 +37,8 @@ case class WorkflowExecuteRequest(
workflowSettings: WorkflowSettings,
emailNotificationEnabled: Boolean,
computingUnitId: Int,
- // The user_warehouse this run writes into; absent = the shared default
warehouse.
- warehouseId: Option[Int]
+ // The user_warehouse row this run writes into; absent = the shared default
+ // warehouse. Named `whid` after the column, so it is not read as
Lakekeeper's
+ // own warehouse id (a UUID).
+ whid: Option[Int]
Review Comment:
I don't think this change is necessary. If you want to change the name, give
a better name.
##########
amber/src/main/scala/org/apache/texera/web/service/ExecutionsMetadataPersistService.scala:
##########
@@ -73,7 +73,7 @@ object ExecutionsMetadataPersistService extends LazyLogging {
// Set computing unit ID if provided
newExecution.setCuid(computingUnitId)
// The warehouse this run writes into (#6870); null = the shared default
warehouse.
- warehouseId.foreach(whid => newExecution.setWhid(whid))
+ whid.foreach(whid => newExecution.setWhid(whid))
Review Comment:
I think having two whid name is so confusing. Simply revert the changes or
give a better name if you want to change it.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
Review Comment:
Maybe update the comment because the catalog name is contradicting.
--
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]
