Skip to content
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-3879] Xa recovery scan cursor handling #2246

Merged
merged 2 commits into from
May 3, 2024

Conversation

marcosgopen
Copy link
Member

@marcosgopen marcosgopen commented Apr 22, 2024

https://issues.redhat.com/browse/JBTM-3879

Patch XA recovery scan cursor handling
Add unit test to test the XAResourceWrapper and Recovery scan status

CORE !TOMCAT !AS_TESTS !RTS !JACOCO !XTS QA_JTA !QA_JTS_OPENJDKORB !PERFORMANCE !LRA !DB_TESTS

@marcosgopen marcosgopen marked this pull request as draft April 22, 2024 15:44
@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@marcosgopen marcosgopen marked this pull request as ready for review April 29, 2024 10:31
@@ -400,16 +400,24 @@ public void start(Xid xid, int i) throws XAException {
}
});

int oldCount = recoverCalled;
callFirstPass(xarm, oldCount);
Copy link
Member

@mmusgrov mmusgrov Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have lost some test precision here, before we asserted the actual number of recover calls but now we are just testing that the number is incrementing on the first path and I'd expect the test would fail if you called callSecondPass twice in a row. Since the expectation on the error free path is two recover calls (TMSTARTRSCAN and TMENDRSCAN) on the first pass and no recover calls on the second pass, the following would be more maintainable:

        int oldCount = recoverCalled;
        callFirstPass(xarm, oldCount + 2);
        callSecondPass(xarm, oldCount + 2);
        callFirstPass(xarm, oldCount + 4);
        callFirstPass(xarm, oldCount + 6);
        callSecondPass(xarm, oldCount + 6);

or better still, because the following makes it obvious what the test is doing:

        int oldCount = recoverCalled;
        // on the error free path
        // we expect periodicWorkFirstPass to make two recover calls (TMSTARTRSCAN and TMENDRSCAN),
        // and we expect periodicWorkSecondPass to make no recover calls
        xarm.periodicWorkFirstPass();
        assertEquals(recoverCalled, oldCount + 2);
        xarm.periodicWorkSecondPass();
        assertEquals(recoverCalled, oldCount + 2);
        xarm.periodicWorkFirstPass();
        assertEquals(recoverCalled, oldCount + 4);
        xarm.periodicWorkFirstPass();
        assertEquals(recoverCalled, oldCount + 6);
        xarm.periodicWorkSecondPass();
        assertEquals(recoverCalled, oldCount + 6);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mmusgrov , I will do that.

@@ -525,20 +533,17 @@ public XAResource[] getXAResources() throws Exception {
// 1st pass: returns one xid (count is 1)
xarm.periodicWorkFirstPass();
// 2nd pass: throws an exception (count is 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are no longer valid because the second pass does not call recover(). Also the following comments stating the value of count are no longer valid. And instead of incrementing recoverCount by 2 each time sometimes it will only increment by 1 or zero if exceptions are thrown. Also it might be worthwhile using recoverCount instead of count in the dummy XAResource so that the test can continue asserting that the recover calls are called the correct number of times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is now very confusing and may be deleted. However I will try to change it.

@marcosgopen
Copy link
Member Author

marcosgopen commented Apr 30, 2024

I tried to change the unit tests with the new commit as you suggested @mmusgrov. WDYT?
I'll then squash the commits.
I simplified the second unit test and set the recoveryCount as global in order to assert on that too, I think it might help understanding the recovery scan logic.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Copy link
Member

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main change request is the method that tests the recovery module with exceptions ie testRecoverPassFailure.

@@ -57,6 +57,7 @@
public class XARecoveryModuleUnitTest
{
private boolean rolledback;
private int recoverCount = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other similar variable is declared at the end of the class, can't you declare them both in the same place? And can't you also use the same variable for the two tests initialising them in the method. Another option is to make the tests independent using a method local variable:

public void testCanRepeatFirstPass () {
   final int[] recoverCalled = new int [1];
   ...
      recoverCalled[0] += 1;
   etc

But personally, to minimise the change set, I'd just reuse recoverCalled for the second test since we cannot run these surefire tests in parallel.

Also i would initialise the counters in the methods that use them since it makes the tests brittle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also doubting that recoverCount needs to be declared as an instance variable: the asserts for the value don't add anything since recoverCount is internal to how the test operates, the code comments about recoverCount should be sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, so I will restore the previous variable (count) and just rename it to 'recoverCount'.

count++;
if (count == 1 || count == 5) {
recoverCount++;
if (recoverCount == 1 || recoverCount == 2) {
Copy link
Member

@mmusgrov mmusgrov Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is no longer the same as the original code and it is testing something different. The original generated an exception on the first two cycles and an array of 1 xid on the third cycle. So I think the recover logic needs to be as follows:

                public Xid[] recover(int i) throws XAException {
                    recoverCount += 1;
                    if (recoverCount == 1 || recoverCount == 3 || recoverCount == 5) {
                        return new Xid[]{xid}; // TMSTARTRSCAN
                    } else if (recoverCount > 5) {
                        return new Xid[0]; // TMSENDRSCAN when recoverCount == 6 so the recovery pass succeeds
                    } else {
                        throw new XAException(); // if recoverCount is 2 or 4 ie the TMSENDRSCAN call
                    }
                }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mmusgrov , PR updated

return new Xid[]{xid};
} else if (count > 5) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a code formatting change that doesn't flow with the formatting around it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

return new Xid[]{xid};
} else if (count > 5) {
}
else if (recoverCount > 3) {
return new Xid[0];
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else arm is unreachable since recoverCount starts out at 1 and the final else is (recoverCount > 3), ie the code is not very maintainable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW in my other comment I suggested an alternative implementation of the recover method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Copy link
Member

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had two minor change requests but the PR is looking good.

Can you also squash the four commits for the test changes - in an ideal world we'd want the tests to be with the functional change but I think it is useful taking Jonathan's commit as is.

@@ -441,8 +446,7 @@ public String getProductVersion() {
public String getJndiName() {
return "test";
}

int count = 0;
int recoverCount = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you have gone back to the original use of this variable I think it should keep the original name - in the future it will make code diffs simpler to analyse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated

recoverCount++;
if (recoverCount == 1 || recoverCount == 3 || recoverCount == 5) {
return new Xid[] { xid }; // TMSTARTRSCAN
}else if (recoverCount > 5) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code reformat has dropped the space between } and else which makes it inconsistent with the rest of the code in this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@marcosgopen marcosgopen changed the title Xa recovery scan [JBTM-3879] Xa recovery scan cursor handling May 2, 2024
@marcosgopen
Copy link
Member Author

marcosgopen commented May 2, 2024

@mmusgrov I have created the JBTM (feel free to add comments/update the description) for it and squash the commits as suggested.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@@ -441,7 +446,6 @@ public String getProductVersion() {
public String getJndiName() {
return "test";
}

int count = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changing it back to count. Quite a few comments still refer to recoverCount, will you change those to count too please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I forgot about that. Updated, thanks

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants