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

control-service: code expected to run in transaction now runs in transaction #2117

Merged
merged 10 commits into from
May 25, 2023

Conversation

murphp15
Copy link
Collaborator

@murphp15 murphp15 commented May 23, 2023

Why

Often the test DataJobTerminationStatusIT fails with the following stacktrace.

org.opentest4j.AssertionFailedError: The value of the taurus_datajob_termination_status metrics does not match. It was actually taurus_datajob_termination_status{data_job="it-datajobterminationstatusit-b1725dc6",execution_id="it-datajobterminationstatusit-b1725dc6-1684778794",service="data-jobs",} 5.0 ==> expected: <true> but was: <false>
	at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at app//org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:211)
	at app//com.vmware.taurus.datajobs.it.DataJobTerminationStatusIT.testDataJobTerminationStatus(DataJobTerminationStatusIT.java:131)
	at 

I was looking into it and I realised the database operations were not actually taking place in a transaction despite the method being marked as transactional.
It is because calling a method from another method in a the same class stops spring from being able to intercept the function call and start a transaction.

Please see here for more details: https://stackoverflow.com/questions/3423972/spring-transaction-method-call-by-the-method-within-the-same-class-does-not-wo

What

In this PR I create a second class to call the transactional method so it can run in a transaction.

How was this tested

Locally, I was not able to reproduce the randomly failing test. But that is not to say I have 100% fixed it.
But this change is important because the code now does what the original author expected and will avoid cryptic bugs in the future.

murphp15 and others added 3 commits May 23, 2023 15:42
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
@murphp15 murphp15 marked this pull request as draft May 23, 2023 14:46
pre-commit-ci bot and others added 7 commits May 23, 2023 14:50
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
@murphp15 murphp15 changed the title [DRAFT] Person/murphp15/fix failing test control-service: code expected to run in transaction now runs in transaction May 24, 2023
@murphp15 murphp15 marked this pull request as ready for review May 24, 2023 13:56
@murphp15 murphp15 requested review from antoniivanov and mivanov1988 and removed request for antoniivanov May 25, 2023 08:07
Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

How did you verify that the method is now executed within a transaction ?

(could be by looking at logs by setting logging.level.org.springframework.transaction=debug; or debugger or best with unit tests).

@murphp15
Copy link
Collaborator Author

@tozka
I did this (could be by looking at logs by setting logging.level.org.springframework.transaction=debug; or debugger or best with unit tests).

@murphp15 murphp15 merged commit 4fe3ef8 into main May 25, 2023
12 checks passed
@murphp15 murphp15 deleted the person/murphp15/fix_failing_test branch May 25, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants