-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix http-push-connection-dispatcher config overrides #2139
base: master
Are you sure you want to change the base?
fix http-push-connection-dispatcher config overrides #2139
Conversation
properties were not in the right place and the config was defaulting to https://github.com/apache/pekko/blob/2469f729f7503acf814bcbd042b4bb0863103c9d/actor/src/main/resources/reference.conf#L506 Signed-off-by: Aleksandar Stanchev <aleksandar.stanchev@bosch.com>
core-pool-size-factor = 4 | ||
core-pool-size-factor = ${?HTTP_PUSH_CORE_POOL_SIZE_FACTOR} | ||
core-pool-size-max = 64, | ||
core-pool-size-max = ${?HTTP_PUSH_CORE_POOL_SIZE_MAX} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You adjusted the defaults.
The prior defaults seemed not logical to me .. but did you test what effect those configs have with e.g. different load?
Might be worth to add a little documentation for those configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior defaults were not working at all as those properties should be inside "thread-pool-executor" object. If putted in such a threadpool with int.max number of threads was being created.
I did tested with different load and was able to publish more than ~200 messages to an endpoint that responds after 1.5s only by changing the dispatcher settings and parallelism of the connection. Before changing the dispatcher config there were "There are too many in-flight requests." (handled by parallelism) and "Dropped message as result of backpressure strategy!" (handled by tweaking the dispatcher). Maybe the default factor can be left at 3 but i don't see a drawback if set to 4. Nevertheless i don't have a strong opinion on the default value as i will be overriding those later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
If you already know you want/need to overwrite them, you could give a hint in the config documentation about how and when to tweak them (e.g. when inspecting certain dispatcher metrics).
But I assume this is highly dependent on the use-case :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you found the root cause 👍
properties were not in the right place and the config was defaulting to https://github.com/apache/pekko/blob/2469f729f7503acf814bcbd042b4bb0863103c9d/actor/src/main/resources/reference.conf#L506
fixes #2138