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

Closes #2374: Deadlock when creating HistoryEvents from many connecti… #2627

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

gitgoodjhe
Copy link
Contributor

@gitgoodjhe gitgoodjhe commented Jul 30, 2024

…ons simultaneously

-Write operations are now performed with the regular TaskanEngine and its' SqlSession and TransactionFactory which provides the needed transactionality and doesn't open multiple connections

Some additional info on this PR:

for this new check in the TaskanaEngineImpl:
if (transactionFactory.getClass().getSimpleName().equals("SpringManagedTransactionFactory")) {
sessionManager.startManagedSession();
}

SONAR would like to use "instanceof" instead of comparing the class name with a String. Problem is, that we have no dependency at this point in the core, so the SpringManagedTransactionFactory.class is not known

The JobScheduler gets initialized in the TaskanaEngineImpl. Unfortunately, it currently creates its' own TaskanaEngineImpl:
if (this.taskanaConfiguration.isJobSchedulerEnabled()) {
TaskanaConfiguration configuration =
new TaskanaConfiguration.Builder(this.taskanaConfiguration)
.jobSchedulerEnabled(false)
.build();
TaskanaEngine taskanaEngine =
TaskanaEngine.buildTaskanaEngine(configuration, EXPLICIT, transactionFactory);

Issue: The HistoryEventManager (and all other SPis) initialization logic follows at the end of the TaskanaEngineImpl constructor. Therefore the initialize() will get called twice and therefore we need the if checks when adding the mappers in the new initialization logic of the SimpleHistoryServiceImpl, e.g.:
if (!sqlSession
.getConfiguration()
.getMapperRegistry()
.hasMapper(TaskHistoryEventMapper.class)) {
sqlSession.getConfiguration().addMapper(TaskHistoryEventMapper.class);
}


Release Notes:


For the submitter:

Verified by the reviewer:

  • Commit message format → (Closes) #<Issue Number>: Your commit message.
  • Submitter's update to documentation is sufficient
  • SonarCloud analysis meets our standards
  • Update of the current release notes reflects changes
  • PR fulfills the ticket
  • Edge cases and unwanted side effects are tested
  • Readability

…onnections simultaneously

-Write operations are now performed with the regular TaskanEngine and its' SqlSession and TransactionFactory which provides the needed transactionality and doesn't open multiple connections
Copy link
Contributor

@CRoberto1926 CRoberto1926 left a comment

Choose a reason for hiding this comment

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

I tested a bit and found no regressions. Good job!

@gitgoodjhe
Copy link
Contributor Author

I tested a bit and found no regressions. Good job!

Hi @CRoberto1926 thank you for your review! I answered in the comment section to your questions. If you agree with the ones where I don't intend to change them, could you please mark them as "resolved"?

@CRoberto1926
Copy link
Contributor

@holgerhagen I can't approve or resolve comments. For me everything is fine, can you please approve and resolve all the comments? Thanks!

@gitgoodjhe gitgoodjhe merged commit 46ab785 into Taskana:master Aug 7, 2024
42 checks passed
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