deepakpanda93 commented on code in PR #19494:
URL: https://github.com/apache/hudi/pull/19494#discussion_r3713490213
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodiePartitionMetadata.java:
##########
@@ -106,22 +106,37 @@ public void trySave() throws HoodieIOException {
RetryHelper<Void, HoodieIOException> retryHelper = new RetryHelper(1000,
3, 1000, HoodieIOException.class.getName())
.tryWith(() -> {
if (!storage.exists(metaPath)) {
- if (format.isPresent()) {
- writeMetafileInFormat(metaPath, format.get());
- } else {
- // Backwards compatible properties file format
- try (ByteArrayOutputStream os = new ByteArrayOutputStream()) {
- props.store(os, "partition metadata");
- Option<byte []> content = Option.of(os.toByteArray());
- storage.createImmutableFileInPath(metaPath,
content.map(HoodieInstantWriter::convertByteArrayToWriter));
+ try {
+ writeMetafile(metaPath);
+ } catch (IOException | HoodieIOException e) {
+ // The metafile is immutable, so tasks writing to the same
partition concurrently race
+ // to create it and all but the winner fail with an 'already
exists' error. Losing that
+ // race means the metafile is in place, which is what the caller
asked for: retrying
+ // would only re-observe the same file, and warning about it is
pure noise.
+ if (!storage.exists(metaPath)) {
Review Comment:
Good catch, this is a real defect in what the PR adds — fixed in
`2c12664e9f0a`.
Chasing it down turned the concern from "could throw" into something
concrete. `RetryHelper` here is built with `HoodieIOException.class.getName()`,
so `retryExceptionsClasses` is exactly `[HoodieIOException]`, and `start()`
does:
```java
} catch (Exception e) {
if (!checkIfExceptionInRetryList(e)) {
throw e; // straight out, no retry
}
```
The write failure out of `createImmutableFileInPath` is a
`HoodieIOException`, so it is retryable. A bare `IOException` from the
existence check is not. So a single failed check did not merely lose the
original: it downgraded a retryable write failure into one thrown immediately,
which on an object store is precisely when the retry was worth having.
The check now cannot replace the failure it was meant to soften:
```java
private boolean metafileExistsAfterFailedWrite(StoragePath metaPath,
Exception writeFailure) {
try {
return storage.exists(metaPath);
} catch (IOException checkFailure) {
writeFailure.addSuppressed(checkFailure);
return false;
}
}
```
A check that cannot answer reports the metafile as absent, so the original
failure is thrown with its retry classification intact, and the check failure
is kept as suppressed rather than dropped.
Covered by `testTrySaveKeepsWriteFailureWhenExistenceRecheckFails`, which
stubs `exists` to slip through the pre-write check and then fail the recheck,
and asserts the thrown exception is not the check failure and that the check
failure survives as suppressed. Verified it is bound to the fix: reverting to
the direct `storage.exists(metaPath)` fails exactly that test with `expected:
not same but was: <java.io.IOException: storage unavailable>`, and leaves the
other seven passing.
Worth noting for anyone reading the test: the stub alternates rather than
using a fixed sequence, because the write failure is retryable. With a
`doReturn(false).doThrow(...)` sequence, Mockito repeats the last stub once
exhausted, so the second attempt throws from the *pre-write* check, which sits
outside the try and escapes for a different reason entirely.
--
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]
