-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JBTM-3893] XARecoveryModule now implements RecoveryModule.hasWorkLeftToDo() #2336
base: main
Are you sure you want to change the base?
Conversation
CI was disabled as |
@@ -412,7 +419,6 @@ protected XARecoveryModule(XARecoveryResourceManager recoveryClass, String logNa | |||
_logName = logName; | |||
_recoveryManagerClass = recoveryClass; | |||
if (_recoveryManagerClass == null) { | |||
this.setRecoveryProblems(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you explain why this is no longer classed as a recovery problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this is not a problem during recovery but rather a problem during the initialisation of Narayana.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus, as setRecoveryProblems(true)
is invoked in the constructor, as soon as periodicWorkFirstPass()
is invoked from PeriodicRecovery
, recoveryProblems
will be set to false (i.e. setRecoveryProblems(false)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if is null then we will eventually end up in https://github.com/jbosstm/narayana/blob/main/ArjunaJTA/jta/classes/com/arjuna/ats/internal/jta/recovery/arjunacore/XARecoveryModule.java#L211 because of a NullPointerException at https://github.com/jbosstm/narayana/blob/main/ArjunaJTA/jta/classes/com/arjuna/ats/internal/jta/recovery/arjunacore/XARecoveryModule.java#L198 so that would make setRecoveryProblems(true)
.
I am not super sure I agree that an invalid configuration should not be considered a problem for recovery but we can think more on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, recoveryProblem
shouldn't be used to report problems during initialisation (at least, as far as I can tell from other use cases). Moreover, before we end up at line 211, we hit setRecoveryProblems(false), which makes setting recoveryProblems
to true during initialisation pointless
@@ -293,6 +295,11 @@ public String id() { | |||
return "XARecoveryModule:" + _recoveryManagerClass; | |||
} | |||
|
|||
@Override | |||
public boolean hasWorkLeftToDo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can I request some tests similar to https://github.com/jbosstm/narayana/blob/main/ArjunaCore/arjuna/tests/classes/com/hp/mwtests/ts/arjuna/recovery/RecoverySuspendTest.java to be added for the XARecoveryModule work left to do checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sure, I was already working on testing but I wanted to start a Draft
PR ASAP. Thanks for asking 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
JBTM-3893
CORE AS_TESTS RTS !JACOCO !XTS QA_JTA QA_JTS_OPENJDKORB !PERFORMANCE DB_TESTS mysql db2 postgres oracle