PDGGK commented on PR #9481: URL: https://github.com/apache/paimon/pull/9481#issuecomment-5549954813
Fixed in the head just pushed — `catch (RuntimeException | Error e)`. One thing I would rather state than leave implied: I did not add a regression for this one, and the existing test does not cover it. I checked rather than assumed — reverting the catch to `RuntimeException` alone leaves `LuminaFileBackedDatasetCloseTest` green, so it has no discriminating power over this change. `FileBackedDataset`'s constructor takes only `File`, `int`, `long`, `String`, `int`, so there is no collaborator to inject an `Error` through, the way `BTreeIndexWriter` has a `KeySerializer`. The only ways to make `allocateDirect` actually throw are to bound `-XX:MaxDirectMemorySize` for the fork, which would make the test pass only under a flag CI does not set, or to request a size large enough to exhaust the default limit, which is machine-dependent and slow. I would rather ship the one-line fix uncovered than add a test that is skipped or flaky in CI. If you would prefer a seam — extracting the buffer allocation into an overridable method, or widening the package-private constructor to take a supplier — say which and I will add it with a proper regression. `spotless:check` and `checkstyle:check` clean; `LuminaFileBackedDatasetCloseTest` still passes. -- 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]
