Yicong-Huang commented on PR #8978: URL: https://github.com/apache/texera/pull/8978#issuecomment-6334052464
> ## Review summary > Test-only change that stops `FileScanSourceOpExecSpec` from leaking a real S3 object into the developer's `rustfs_data` volume on every local run. It deletes the uploaded object in the test's existing outer `finally` (via `deleteByExecution`, which logs rather than throws, so it can't displace the test's own failure), asserts the prefix is empty afterward, and moves the write prefix off `8888` to a negative id so the prefix-delete can never touch a real execution's binaries (real EIDs are positive `SERIAL`). > > The change is sound. The write (`baseUriForExecution`), delete (`executionPrefix`), and assertion all resolve to the same `objects/-8670/` prefix, and both `S3StorageClient.directoryExists` and `deleteDirectory` add the trailing `/` only when missing, so there is no double-slash and `-8670/` cannot match `-86700/` -- the boundary the description relies on holds. The fixed-id vs random-id tradeoff for two checkouts sharing one RustFS is already weighed in the PR description and is a reasonable call for debuggability; no objection. No blocking issues. > > **Action items:** Nothing blocking; the single P3 below is optional polish. > > ## Findings > **1 total: 0 P0, 0 P1, 0 P2, 1 P3.** > > ### Nit (P3) > * **Tie the produced pointer to the cleaned prefix** - `common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/scan/file/FileScanSourceOpExecSpec.scala:193` - see inline. > > ## Verification > * No blocking findings to substantiate. Traced the three paths and confirmed they agree on `objects/-8670/`: write base URI `baseUriForExecution(-8670)` = `s3://<bucket>/objects/-8670/` with `create()` appending a UUID; cleanup `deleteByExecution(-8670)` deletes `executionPrefix(-8670)` = `objects/-8670` (normalized to `objects/-8670/`); the assertion checks `directoryExists(DEFAULT_BUCKET, "objects/-8670/")`. Both S3 helpers normalize the trailing slash, so no `//` and no prefix match against `-86700/`. Negative id is safe because `workflow_executions.eid` is positive `SERIAL`. > * Did not run the suite (no S3 in this environment). Relied on the author's documented red/green local runs for the empty-bucket result; statically verified the prefix arithmetic and the negative-id safety argument. That looks like the one we run on spark lol -- 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]
