dongjoon-hyun commented on PR #58312: URL: https://github.com/apache/spark/pull/58312#issuecomment-5429553235
Thanks for tracking this down. I walked through the four `release()` cases (`delete` x `active` membership) against the old code and the accounting outcome is preserved in each, so the change looks correct to me. Folding the deduction and the store operation under the `active` lock is also consistent with what `Lease.commit` and `openStore` already do in this class. One thing worth calling out in the description: the `makeRoom()` re-check also fixes a second bug on its own -- previously a candidate collected before a concurrent `openStore()` could be deleted while the UI was holding the path. I also checked the lock ordering. `release()` now does `listing` I/O under the `active` lock, while `makeRoom()` acquires `active` while holding an open listing iterator. That crossing already exists on master, and `LevelDB` writes only take the per-type monitor (not held by iterators), so I don't see a deadlock cycle. A few minor comments: 1. In `release()`, the comment `// If the app was not actively tracked, its size was not deducted above; do it now.` is now stale -- there is no unconditional deduction above it anymore. 2. In the final `else if (oldSizeOpt.isDefined)` branch, the example in the comment (`evicted by a concurrent makeRoom()`) is exactly the case this PR eliminates: `makeRoom()` now skips active apps under the lock. The branch is still worth keeping as a defensive path (out-of-band deletion, a partially failed `deleteRecursively`), but the justification could be reworded. 3. When `makeRoom()` skips a candidate because its directory is gone, the `ApplicationStoreInfo` entry stays in the listing, since `deleteStore()` is what removes it. The window is narrow, but such an entry is permanent and gets counted against `needed` on every later `makeRoom()`, which under-evicts. A `listing.delete(classOf[ApplicationStoreInfo], info.path)` in that branch would clean it up. Optional -- it does widen the scope a bit. 4. `release(delete = true)` now holds the global `active` lock across the recursive store deletion, and `FsHistoryProvider.cleanLogs` calls it in a loop. `openStore()` from UI requests blocks for that duration. Not a new class of problem given the existing `Lease.commit` behavior, but it may be worth a note in the description that this is a deliberate trade-off. 5. In the test, after the fix the main thread blocks on the `active` lock inside `openStore()`, so `openStoreDone.countDown()` in the `finally` is never reached and the `await(2, TimeUnit.SECONDS)` inside the `sizeOf` stub always runs to timeout. The test is correct, but it costs a fixed 2 seconds on every run. 6. Nit: `new File(info.path)` is constructed twice in `makeRoom()`; a local val would do. Finally, since SPARK-56044 shipped in 4.2.0 / 4.1.2 / 4.0.3, should this be backported to those branches as well? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
