kunwp1 commented on code in PR #7745:
URL: https://github.com/apache/texera/pull/7745#discussion_r3810607259
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -94,15 +126,18 @@ class WarehouseResource(client: LakekeeperClient, enabled:
Boolean) extends Lazy
if (!enabled) {
return WarehouseStatus(enabled = false, warehouses = List())
}
- val warehouses = context
+ val rows = context
.selectFrom(USER_WAREHOUSE)
.where(USER_WAREHOUSE.UID.eq(current_user.getUid))
.orderBy(USER_WAREHOUSE.CREATED_AT.asc())
.fetch()
- .map(row => toDashboardWarehouse(row))
+ .asScala
+ .toList
+ val owners = resolveOwners(rows.map(_.getUid).distinct)
WarehouseStatus(
enabled = true,
- warehouses = warehouses.toArray(Array[DashboardWarehouse]()).toList
+ warehouses =
+ rows.map(row => toDashboardWarehouse(row, owners.getOrElse(row.getUid,
(null, null))))
Review Comment:
Name the variable for `(null, null)` to something like noOwner.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -53,16 +54,47 @@ object WarehouseResource {
name: String,
warehouseName: String,
flavor: String,
- createdAtMillis: Long
+ createdAtMillis: Long,
+ // Owner display info, mirroring DashboardWorkflowComputingUnit: today
every
+ // warehouse belongs to the caller, but the UI binds to the entry rather
than
+ // the session user so shared warehouses render the right person (#7743).
+ ownerName: String,
+ ownerAvatar: String
)
- private def toDashboardWarehouse(row: UserWarehouseRecord):
DashboardWarehouse =
+ // (name, avatar) per uid; null when the user has no name / avatar set,
matching
+ // how computing units resolve their owner info.
+ private def resolveOwners(uids: Seq[Integer]): Map[Integer, (String,
String)] =
+ if (uids.isEmpty) Map.empty
+ else
+ context
+ .select(USER.UID, USER.NAME, USER.AVATAR)
+ .from(USER)
+ .where(USER.UID.in(uids: _*))
+ .fetch()
+ .asScala
+ .map(r =>
+ r.get(USER.UID) -> (
Review Comment:
redundant parentheses.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -94,15 +126,18 @@ class WarehouseResource(client: LakekeeperClient, enabled:
Boolean) extends Lazy
if (!enabled) {
return WarehouseStatus(enabled = false, warehouses = List())
}
- val warehouses = context
+ val rows = context
.selectFrom(USER_WAREHOUSE)
.where(USER_WAREHOUSE.UID.eq(current_user.getUid))
.orderBy(USER_WAREHOUSE.CREATED_AT.asc())
.fetch()
- .map(row => toDashboardWarehouse(row))
+ .asScala
+ .toList
+ val owners = resolveOwners(rows.map(_.getUid).distinct)
Review Comment:
You can combine these two queries into a single query using join. It's more
efficient because it reduces one round trip
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -169,7 +204,7 @@ class WarehouseResource(client: LakekeeperClient, enabled:
Boolean) extends Lazy
}
throw new WebApplicationException(e.getMessage, 500)
}
- toDashboardWarehouse(row)
+ toDashboardWarehouse(row, resolveOwners(Seq(uid)).getOrElse(uid, (null,
null)))
Review Comment:
It's unnecessary to query the database.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -53,16 +54,47 @@ object WarehouseResource {
name: String,
warehouseName: String,
flavor: String,
- createdAtMillis: Long
+ createdAtMillis: Long,
+ // Owner display info, mirroring DashboardWorkflowComputingUnit: today
every
+ // warehouse belongs to the caller, but the UI binds to the entry rather
than
+ // the session user so shared warehouses render the right person (#7743).
+ ownerName: String,
+ ownerAvatar: String
)
- private def toDashboardWarehouse(row: UserWarehouseRecord):
DashboardWarehouse =
+ // (name, avatar) per uid; null when the user has no name / avatar set,
matching
+ // how computing units resolve their owner info.
+ private def resolveOwners(uids: Seq[Integer]): Map[Integer, (String,
String)] =
+ if (uids.isEmpty) Map.empty
+ else
+ context
+ .select(USER.UID, USER.NAME, USER.AVATAR)
+ .from(USER)
+ .where(USER.UID.in(uids: _*))
+ .fetch()
Review Comment:
You can simplify this with `UserDao(context.configuration).fetchByUid(uids:
_*)`
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:
##########
@@ -53,16 +54,47 @@ object WarehouseResource {
name: String,
warehouseName: String,
flavor: String,
- createdAtMillis: Long
+ createdAtMillis: Long,
+ // Owner display info, mirroring DashboardWorkflowComputingUnit: today
every
+ // warehouse belongs to the caller, but the UI binds to the entry rather
than
+ // the session user so shared warehouses render the right person (#7743).
+ ownerName: String,
+ ownerAvatar: String
)
- private def toDashboardWarehouse(row: UserWarehouseRecord):
DashboardWarehouse =
+ // (name, avatar) per uid; null when the user has no name / avatar set,
matching
+ // how computing units resolve their owner info.
+ private def resolveOwners(uids: Seq[Integer]): Map[Integer, (String,
String)] =
Review Comment:
I read your comment and I just feel like it should go to `common/dao` so
that both computing unit managing service and this can reuse it.
--
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]
