hudi-agent commented on code in PR #19494:
URL: https://github.com/apache/hudi/pull/19494#discussion_r3712694250
##########
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:
🤖 This re-check `storage.exists(metaPath)` can itself throw `IOException`
(e.g. transient throttling on an object store — plausible right when the write
just failed transiently). If it throws, the original `e` is lost and a raw
`IOException` propagates instead. That new exception isn't in this
RetryHelper's retry list (only `HoodieIOException` is), so the original
retryable write failure no longer gets retried, and a checked `IOException` can
slip past callers that catch only `HoodieIOException`. Would it be safer to
rethrow the original `e` when the existence re-check fails, so a genuine
transient write error still flows through the normal retry path?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/test/java/org/apache/hudi/common/util/TestRetryHelper.java:
##########
@@ -52,6 +61,55 @@ public void testCheckIfExceptionInRetryList() throws
Exception {
assertTrue(retry);
}
+ /**
+ * The point of HUDI-9095: a retry that is going to be attempted again must
not dump a stack trace
+ * into the log. The cause still has to be identifiable from the message
itself.
+ */
+ @Test
+ public void testRetryWarningCarriesNoStackTrace() {
+ HoodieTestLogAppender appender = new
HoodieTestLogAppender().attachTo(RetryHelper.class);
+ try {
+ AtomicInteger attempts = new AtomicInteger(0);
+ RetryHelper retryHelper = new RetryHelper(INTERVAL_TIME, 3,
INTERVAL_TIME, (String) null, "save partition metafile");
+ assertDoesNotThrow(() -> retryHelper.start(() -> {
+ if (attempts.incrementAndGet() < 3) {
+ throw new IOException("Failed to create file
/a/b/.hoodie_partition_metadata",
+ new FileAlreadyExistsException("File already exists:
/a/b/.hoodie_partition_metadata"));
+ }
+ return true;
+ }));
+
+ List<LogEvent> warnings = appender.getLog().stream()
+ .filter(event ->
Level.WARN.equals(event.getLevel())).collect(Collectors.toList());
+ assertFalse(warnings.isEmpty(), "the retries should still be reported at
warn level");
+ for (LogEvent warning : warnings) {
+ assertNull(warning.getThrown(),
+ "the retry warning must not carry a throwable, otherwise the
logger prints its stack trace");
+ String message = warning.getMessage().getFormattedMessage();
+ assertTrue(message.contains("save partition metafile"), message);
+ assertTrue(message.contains("java.io.IOException: Failed to create
file /a/b/.hoodie_partition_metadata"), message);
+ assertTrue(message.contains("FileAlreadyExistsException"), "the root
cause must survive: " + message);
+ }
+ } finally {
+ appender.detach();
+ }
+ }
+
+ @Test
Review Comment:
🤖 nit: could you rename this to `testSummarizeKeepsRootCauseOnASingleLine`
to match the spelling of the production method `summarize`? The
British-vs-American mismatch is a small speed-bump when scanning for which test
covers which method.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
