sahibamatta commented on PR #1866: URL: https://github.com/apache/incubator-uniffle/pull/1866#issuecomment-2231993045
> When an exception happens during the flushing process, just let `commitShuffle` know, make it fail. Hi made the changes in the PR. So now, when the `storageManager.write(..)` operation fails inside the `processFlushEvent` method, a `writeError` variable is set. When the `commitShuffle` runs, it calls the `getCommittedBlockIds`, which will first check the `writeError` variable, if found set it will throw: `EventDiscardException`. There a few things to note/questions though: - For now, the `commitShuffle()` will only fail, when the `write()` operation inside the `processFlushEvent()` fails. Do you think the logic looks fine and we should extend it for the entire `processFlushEvent()` i.e. fail `commitShuffle` , whenever there is any failure/exception inside the `processFlushEvent()` method? - Throwing exceptions from constructor, puts `ShuffleFlushManager` in an undesirable state. Do you think we should instead delay throwing the exception, when a method like `getCommittedBlockIds()` gets called? Something similar to what I did in my current implementation? - I didn't add any UTs as of now, as I'm unsure how to generate a test scenario for this. We may need a mechanism to set `writeError` through UT so that we can assert `commitShuffle()` failing when `write` operation fails. -- 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]
