You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2025/12/19 16:10:16 UTC
[PR] Fixed possible flaky test issue in FateITBase [accumulo]
dlmarion opened a new pull request, #6032:
URL: https://github.com/apache/accumulo/pull/6032
Modified a condition in the test to account for the case where the transaction runner thread may or may not have started working on the fate operation.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635715318
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
+ // latch to be called to continue.
+ assertTrue(TStatus.SUBMITTED == getTxStatus(sctx, txid)
+ || TStatus.IN_PROGRESS == getTxStatus(sctx, txid));
Review Comment:
I don't think wait is necessary. Once it's seeded it will at least be SUBMITTED, this was just a bit flaky before as it could be IN_PROGRESS if a thread picks it up and works on it before the SUBMITTED check
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635726039
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
Review Comment:
```suggestion
// IN_PROGRESS state, but will be waiting on the callStarted
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635787337
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
Review Comment:
Sorry, yeah you're correct. Nothing to change here
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635722731
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
+ // latch to be called to continue.
+ assertTrue(TStatus.SUBMITTED == getTxStatus(sctx, txid)
+ || TStatus.IN_PROGRESS == getTxStatus(sctx, txid));
Review Comment:
`callStarted.await();` essentially does the wait for us but is a bit cleaner than sleeping
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635688716
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
+ // latch to be called to continue.
+ assertTrue(TStatus.SUBMITTED == getTxStatus(sctx, txid)
+ || TStatus.IN_PROGRESS == getTxStatus(sctx, txid));
Review Comment:
This was based on a discussion with @kevinrr888. I think at this place it's guaranteed to be one or the other, I'm not sure we need a wait. @kevinrr888 ?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635767505
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
Review Comment:
If the fate runner thread has started the op, then it will call `callStarted.countDown()`, which line 603 is going to wait on. Then the fate op will wait on the `finishCall` latch, right?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6032:
URL: https://github.com/apache/accumulo/pull/6032#discussion_r2635653487
##########
test/src/main/java/org/apache/accumulo/test/fate/FateITBase.java:
##########
@@ -591,7 +591,13 @@ protected void testShutdownDoesNotFailTx(FateStore<TestEnv> store, ServerContext
fate.seedTransaction(TEST_FATE_OP, txid, new TestRepo("testShutdownDoesNotFailTx"), true,
"Test Op");
- assertEquals(TStatus.SUBMITTED, getTxStatus(sctx, txid));
+ // The Fate operation could be in a SUBMITTED state if the
+ // Fate transaction runner thread has not picked it up yet.
+ // If the thread has picked it up, then it will be in an
+ // IN_PROGRESS state, but will be waiting on the finishCall
+ // latch to be called to continue.
+ assertTrue(TStatus.SUBMITTED == getTxStatus(sctx, txid)
+ || TStatus.IN_PROGRESS == getTxStatus(sctx, txid));
Review Comment:
Could we do a `Wait.for(()->TStatus.IN_PROGRESS == getTxStatus(sctx, txid))` or is it not guaranteed to reach IN_PROGRESS?
Also were you seeing test failures that prompted this change?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed possible flaky test issue in FateITBase [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #6032:
URL: https://github.com/apache/accumulo/pull/6032
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org