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.

Sign up for GitHub

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

Jump to bottom

Pooled Streaming Event Processor configuration enhancement #2276

Merged
merged 2 commits into from Jul 7, 2022
Merged

Pooled Streaming Event Processor configuration enhancement #2276

merged 2 commits into from Jul 7, 2022

Conversation

smcvb
Copy link
Member

@smcvb smcvb commented Jul 7, 2022

This pull request contains two adjustments around PooledStreamingEventProcessor configuration:

  1. We introduced a usingPooledStreamingEventProcessors method to the EventProcessingConfigurer that takes in the PooledStreamingProcessorConfiguration. With this users can define their default PooledStreamingEventProcessor in one go.
  2. The thread pool size for the Worker Executor is changed to four in the EventProcessingModule. This is inline with the Spring Boot auto configuration, which also defaults to four.

Point one is most definitely a simplification.
Point two is arguably a bug fix, as there's an unintended difference between Java configuration and Spring Boot auto configuration.
Due to point two, this pull request targets the axon-4.5.x instead of master.

smcvb added 2 commits July 7, 2022 12:20
@smcvb
Allows config definition when defaulting to PSEP
25fadb0 
When invoking the
EventProcessingConfigurer#usingPooledStreamingEventProcessors method, it
 would be beneficial to provide the default PSEP configuration as well.
 To that end we can easily introduce a default method that firstly
 invokes usingPooledStreamingEventProcessors and after that invokes
 registerPooledStreamingEventProcessorConfiguration.

#enhancement/psep-config
@smcvb
Define default executors with a pool size
a1c1862 
The Spring Boot Auto Configuration defaults the workerExecutor thread
pool to four, whereas the EventProcessingModule defaults it to one.
Adjust this deviation by letting the EventProcessingModule construct an
executor with a pool size.

#enhancement/psep-config
@smcvb smcvb added Type: Enhancement Use to signal an issue enhances an already existing feature of the project. Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: In Progress Use to signal this issue is actively worked on. labels Jul 7, 2022
@smcvb smcvb added this to the Release 4.5.13 milestone Jul 7, 2022
@smcvb smcvb requested a review from a team July 7, 2022 11:34
@smcvb smcvb self-assigned this Jul 7, 2022
@smcvb smcvb requested review from gklijs, CodeDrivenMitch and YvonneCeelie and removed request for a team July 7, 2022 11:34
CodeDrivenMitch
CodeDrivenMitch reviewed Jul 7, 2022
View reviewed changes
Copy link
Member

@CodeDrivenMitch CodeDrivenMitch left a comment

Choose a reason for hiding this comment

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

The changes look alright. However, I am missing the main reason for the changes (besides just streamlining configuration). Is it related to an issue reported by a user?

@CodeDrivenMitch CodeDrivenMitch self-requested a review July 7, 2022 13:49
@smcvb
Copy link
Member Author

smcvb commented Jul 7, 2022

The changes look alright. However, I am missing the main reason for the changes (besides just streamlining configuration). Is it related to an issue reported by a user?

Yep, this was reported by a user.
They noticed that after the usingPooledStreamingEventProcessors, their Pooled Streaming Event Processors acted synchronously instead of asynchronously.
What happens, is that by defining the use of a PSEP through that property, that the Spring Boot auto configuration didn't have a clue you wanted to use the PSEP at all.
As this was the only spot where we set the Worker Executor thread pool to four, we fell back to the EventProcessingModule's default of one.

As stated, although strictly not a bug, it does deviate from the desired behavior.

@smcvb smcvb merged commit 3990411 into master Jul 7, 2022
@smcvb smcvb deleted the enhancement/psep-config branch July 7, 2022 15:41
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodeDrivenMitch CodeDrivenMitch CodeDrivenMitch approved these changes

@gklijs gklijs Awaiting requested review from gklijs gklijs was automatically assigned from AxonFramework/framework-developers

@YvonneCeelie YvonneCeelie Awaiting requested review from YvonneCeelie YvonneCeelie was automatically assigned from AxonFramework/framework-developers

Assignees

@smcvb smcvb

Labels
Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Milestone
Release 4.5.13
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@smcvb @CodeDrivenMitch