mengw15 commented on code in PR #5171:
URL: https://github.com/apache/texera/pull/5171#discussion_r3296848785
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1065,6 +1065,8 @@ class DatasetResource {
@Auth user: SessionUser
): List[DashboardDataset] = {
val uid = user.getUid
+ // Drop DB rows whose LakeFS repo is missing.
+ val existingRepos = LakeFSStorageClient.listAllRepoNames()
Review Comment:
TOCTOU note: `listAllRepoNames()` is a snapshot taken before the `.map` that
calls `retrieveRepositorySize` per row. If a concurrent admin / orchestrator
deletes a LakeFS repo between the snapshot and the per-row size lookup, the
request will still 500 on the now-stale "exists" check. The window is small but
non-zero.
Worth knowing the existing dataset-search path
(`DatasetSearchQueryBuilder.toEntryImpl` at lines 127-137 on `main`) already
handles this with a `try { retrieveRepositorySize(...) } catch (ApiException) {
return null }` pattern, logging and silently dropping the orphan. After this PR
the two read paths have two different defenses for the same underlying
inconsistency.
Could we use try-catch here too? That would close the race window, drop the
need for the new `listAllRepoNames()` helper entirely, and unify the orphan
defense with the existing search path. What do you think?
--
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]
