[GitHub] [hudi] codope commented on a diff in pull request #5255: [HUDI-3798] Fixing ending of a transaction by different owner and removing some extraneous methods in trxn manager

[GitBox <gi...@apache.org>]

codope commented on code in PR #5255:
URL: https://github.com/apache/hudi/pull/5255#discussion_r845742512


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1432,13 +1433,13 @@ protected final HoodieTable initTable(WriteOperationType operationType, Option<S
     }
 
     HoodieTable table;
-
-    this.txnManager.beginTransaction();
+    HoodieInstant ownerInstant = new HoodieInstant(true, CommitUtils.getCommitActionType(operationType, metaClient.getTableType()), instantTime.get());

Review Comment:
   possible that instantTime is passed Option.empty() by the caller? For `initTable`, it's not likely but let's ensure that.



##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/TestTransactionManager.java:
##########
@@ -152,6 +165,32 @@ public void testMultiWriterTransactions() {
     Assertions.assertTrue(writer2Completed.get());
   }
 
+  @Test
+  public void testEndTransactionByDiffOwner() throws InterruptedException {
+    // 1. Begin and end by the same transaction owner
+    Option<HoodieInstant> lastCompletedInstant = getInstant("0000001");
+    Option<HoodieInstant> newTxnOwnerInstant = getInstant("0000002");
+    transactionManager.beginTransaction(newTxnOwnerInstant, lastCompletedInstant);
+
+    CountDownLatch countDownLatch = new CountDownLatch(1);
+    // Another writer thread
+    Thread writer2 = new Thread(() -> {
+      Option<HoodieInstant> newTxnOwnerInstant1 = getInstant("0000003");
+      transactionManager.endTransaction(newTxnOwnerInstant1);
+      countDownLatch.countDown();
+    });
+
+    writer2.start();
+    countDownLatch.await(30, TimeUnit.SECONDS);
+    // should not have reset the state within transaction manager since the owner is different.
+    Assertions.assertTrue(transactionManager.getCurrentTransactionOwner().isPresent());

Review Comment:
   maybe make reset method just visible for testing?



##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/TestTransactionManager.java:
##########
@@ -152,6 +165,32 @@ public void testMultiWriterTransactions() {
     Assertions.assertTrue(writer2Completed.get());
   }
 
+  @Test
+  public void testEndTransactionByDiffOwner() throws InterruptedException {
+    // 1. Begin and end by the same transaction owner
+    Option<HoodieInstant> lastCompletedInstant = getInstant("0000001");
+    Option<HoodieInstant> newTxnOwnerInstant = getInstant("0000002");
+    transactionManager.beginTransaction(newTxnOwnerInstant, lastCompletedInstant);
+
+    CountDownLatch countDownLatch = new CountDownLatch(1);
+    // Another writer thread
+    Thread writer2 = new Thread(() -> {
+      Option<HoodieInstant> newTxnOwnerInstant1 = getInstant("0000003");
+      transactionManager.endTransaction(newTxnOwnerInstant1);
+      countDownLatch.countDown();
+    });
+
+    writer2.start();
+    countDownLatch.await(30, TimeUnit.SECONDS);
+    // should not have reset the state within transaction manager since the owner is different.
+    Assertions.assertTrue(transactionManager.getCurrentTransactionOwner().isPresent());

Review Comment:
   iiuc, even in this scenario, without this patch, `reset` would be called and it would return w/o resetting but then `unlock` would also be called which we don't want. And this patch fixes this behavior. So, can we verify that not only state is not reset but also `unlock` is not called?



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org