Skip to content

OBGM-584 Fix issue with missing session while executing jobs#4160

Merged
awalkowiak merged 3 commits intofeature/upgrade-to-grails-3.3.10from
OBGM-584
Jul 17, 2023
Merged

OBGM-584 Fix issue with missing session while executing jobs#4160
awalkowiak merged 3 commits intofeature/upgrade-to-grails-3.3.10from
OBGM-584

Conversation

@awalkowiak
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@kchelstowski kchelstowski left a comment

Choose a reason for hiding this comment

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

Just a few questions out of curiosity, to get to know the flow. But overall good job @awalkowiak, it was not an easy task!

@awalkowiak awalkowiak added status: work in progress For issues or pull requests that are in progress but not yet completed warn: do not merge Marks a pull request that is not yet ready to be merged labels Jul 12, 2023
@awalkowiak
Copy link
Collaborator Author

awalkowiak commented Jul 13, 2023

@kchelstowski @jmiranda I added the missing Transactionals and made minor cleanup. I also created a new ticket for creating a SessionAwareJob with proof of concept of it (OBGM-605).

@awalkowiak awalkowiak removed status: work in progress For issues or pull requests that are in progress but not yet completed warn: do not merge Marks a pull request that is not yet ready to be merged labels Jul 13, 2023
Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

This looks fine to me. However, I will say that I would prefer the sessionRequired property to be explicitly added to each job (similar to concurrent property) because it's just a property and there's no functionality associated with it. This would also provide a less opaque solution and would let us control the sessionRequired property for each job in the rare case we needed to deal allow sessions for some of the jobs.

The parent class would have been if we needed to do something like this. But just adding a property seems like overkill.

void execute() {
    doSomethingBefore()
    execute()
    doSomethingAfter()
}

With that said, it seems like a fine solution.

plus wrap executes content with .withNewSession when there is a transaction and a gorm save
@awalkowiak
Copy link
Collaborator Author

@jmiranda I moved the sessionRequired property back to each job, plus if there was a transaction with gorm save in the execute I wrapped its content with withNewSession.

@awalkowiak awalkowiak merged commit b5b7033 into feature/upgrade-to-grails-3.3.10 Jul 17, 2023
@awalkowiak awalkowiak deleted the OBGM-584 branch July 17, 2023 08:56
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.

3 participants