Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix http-push-connection-dispatcher config overrides #2139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions connectivity/service/src/main/resources/connectivity.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,11 +1329,18 @@ http-push-connection-dispatcher {
# This executor is meant to be allowed to grow quite big as its limited by the max parallelism of each http connection client.
# Limit this parallelism here additionally could lead to confusing results regarding througput of some http connections.
# The core-pool-size-min remains unchanged so its quite small (8). Unused threads will expire after 60s.
core-pool-size-factor = 2147483647
core-pool-size-factor = ${?HTTP_PUSH_CORE_POOL_SIZE_FACTOR}

core-pool-size-max = 2147483647
core-pool-size-max = ${?HTTP_PUSH_CORE_POOL_SIZE_MAX}
thread-pool-executor {
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}
Comment on lines +1334 to +1337
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

max-pool-size-min= 64,
max-pool-size-min= ${?HTTP_PUSH_CORE_POOL_SIZE_MAX} # Overriding with the value of HTTP_PUSH_CORE_POOL_SIZE_MAX as if threads are a reason for http problems the pool normaly doesn't grow on its own so
# it is better to set the core upper limit manualy and this config will forse the core pool size to this value.
max-pool-size-max = 2147483647,
allow-core-timeout = on # Enables timeout of core threads
}
}

kafka-consumer-dispatcher {
Expand Down
Loading