voonhous commented on code in PR #19120:
URL: https://github.com/apache/hudi/pull/19120#discussion_r3602235441
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/common/TestMultipleHoodieJavaWriteClient.java:
##########
@@ -207,8 +208,14 @@ record -> {
String startCommitTime = writer.startCommit();
List<WriteStatus> s =
writer.upsert(Collections.singletonList(record), startCommitTime);
- writer.commit(startCommitTime, s);
- LOGGER.info("Completed commit");
+ try {
+ writer.commit(startCommitTime, s);
+ LOGGER.info("Completed commit");
+ } catch (HoodieWriteConflictException e) {
+ // OCC conflict is expected when multiple writers operate
concurrently on overlapping key ranges.
+ // The test verifies no deadlock occurs, not that every commit
succeeds.
+ LOGGER.info("Write conflict on commit {}, expected OCC
behavior: {}", startCommitTime, e.getMessage());
+ }
synchronized (writerQueue) {
Review Comment:
If `commit()` throws anything other than `HoodieWriteConflictException` we
skip the `put()` here and lose that writer -- remaining threads can then block
forever on `take()`. Pre-existing, but a `finally` around the `put()` would
harden it. Heads up: another branch is fixing this same test with exactly that
`finally`, so worth syncing to avoid a clash.
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/common/TestMultipleHoodieJavaWriteClient.java:
##########
@@ -207,8 +208,14 @@ record -> {
String startCommitTime = writer.startCommit();
List<WriteStatus> s =
writer.upsert(Collections.singletonList(record), startCommitTime);
- writer.commit(startCommitTime, s);
- LOGGER.info("Completed commit");
+ try {
+ writer.commit(startCommitTime, s);
+ LOGGER.info("Completed commit");
+ } catch (HoodieWriteConflictException e) {
+ // OCC conflict is expected when multiple writers operate
concurrently on overlapping key ranges.
+ // The test verifies no deadlock occurs, not that every commit
succeeds.
+ LOGGER.info("Write conflict on commit {}, expected OCC
behavior: {}", startCommitTime, e.getMessage());
Review Comment:
Nit: fires on every conflict (up to 20 x 3 writers) -- harmless, just a bit
chatty.
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/common/TestMultipleHoodieJavaWriteClient.java:
##########
@@ -207,8 +208,14 @@ record -> {
String startCommitTime = writer.startCommit();
List<WriteStatus> s =
writer.upsert(Collections.singletonList(record), startCommitTime);
- writer.commit(startCommitTime, s);
- LOGGER.info("Completed commit");
+ try {
+ writer.commit(startCommitTime, s);
+ LOGGER.info("Completed commit");
Review Comment:
Since conflicts are now swallowed and there's no assertion at the end, this
passes as long as it doesn't deadlock -- even if every commit conflicted. Fits
the no-deadlock intent, but tracking successes in an `AtomicInteger` and
asserting `> 0` would keep it able to catch a real regression.
--
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]
