refactor fates storage layer #4017
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactored the storage layer of FATE to return an object when reserving a fate transaction. This object allows mutating the storage related to that FATE transaction. This replaces methods where the fate transaction id had to to always be passed. These changes are a subset of the changes in apache#3964, but only focusing on the storage layer refactoring and nothing else.
cshannon
requested changes
Dec 5, 2023
There was a problem hiding this comment.
The overall structure of the changes look good to me but I tried running the FateIT test and it is failing for me for multiple of the tests so something doesn't seem quite right.
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.accumulo.test.fate.zookeeper.FateIT
[ERROR] Tests run: 4, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 20.61 s <<< FAILURE! -- in org.apache.accumulo.test.fate.zookeeper.FateIT
[ERROR] org.apache.accumulo.test.fate.zookeeper.FateIT.testCancelWhileSubmittedAndRunning -- Time elapsed: 3.695 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
at org.junit.jupiter.api.AssertFalse.failNotFalse(AssertFalse.java:63)
at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:36)
at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:31)
at org.junit.jupiter.api.Assertions.assertFalse(Assertions.java:228)
at org.apache.accumulo.test.fate.zookeeper.FateIT.testCancelWhileSubmittedAndRunning(FateIT.java:279)
...
[ERROR] org.apache.accumulo.test.fate.zookeeper.FateIT.testCancelWhileNew -- Time elapsed: 3.038 s <<< ERROR!
java.lang.IllegalStateException: Can not delete in progress transaction FATE[389879b65476f988]
at org.apache.accumulo.core.fate.Fate.delete(Fate.java:356)
at org.apache.accumulo.test.fate.zookeeper.FateIT.testCancelWhileNew(FateIT.java:241)
...
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] FateIT.testCancelWhileSubmittedAndRunning:279 expected: <false> but was: <true>
[ERROR] Errors:
[ERROR] FateIT.testCancelWhileNew:241 » IllegalState Can not delete in progress transaction FATE[389879b65476f988]
[INFO]
[ERROR] Tests run: 4, Failures: 1, Errors: 1, Skipped: 0
|
@keith-turner - The test failures appear to be race condition related. I took a shot at fixing the tests and pushed a commit up that fixes it. Feel free to revert it or change it if you think it isn't correct of course. After my commit: |
cshannon
approved these changes
Dec 5, 2023
Those changes look good. Thanks for the fix. Thinking that is a pre-existing bug in the test, may be worthwhile back-porting just the test changes. |
keith-turner
added a commit
to keith-turner/accumulo
that referenced
this pull request
Dec 6, 2023
keith-turner
added a commit
to keith-turner/accumulo
that referenced
this pull request
Dec 7, 2023
This change modifies FATE to use singe thread to find work. It also cleans up some of the signaling between threads in FATE and fixes a synchronization bug in FATE that was introduced in apache#4017. The bug introduced in apache#4017 is that somethings are syncronizing on the wrong object because a new inner class was introduced.
keith-turner
added a commit
to keith-turner/accumulo
that referenced
this pull request
Dec 7, 2023
This change modifies FATE to use singe thread to find work. It also cleans up some of the signaling between threads in FATE and fixes a synchronization bug in FATE that was introduced in apache#4017. The bug introduced in apache#4017 is that somethings are syncronizing on the wrong object because a new inner class was introduced. These changes were pulled from apache#3964 and cleaned up and improved.
keith-turner
added a commit
that referenced
this pull request
Dec 8, 2023
This change modifies FATE to use singe thread to find work. It also cleans up some of the signaling between threads in FATE and fixes a synchronization bug in FATE that was introduced in #4017. The bug introduced in #4017 is that somethings are syncronizing on the wrong object because a new inner class was introduced. These changes were pulled from #3964 and cleaned up and improved.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactored the storage layer of FATE to return an object when reserving a fate transaction. This object allows mutating the storage related to that FATE transaction. This replaces methods where the fate transaction id had to to always be passed.
These changes are a subset of the changes in #3964, but only focusing on the storage layer refactoring and nothing else.