Skip to content

[ADDED] MQTT: allow custom timeout for JS API calls #6833

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

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

Conversation

zlymeda
Copy link

@zlymeda zlymeda commented Apr 24, 2025

There was a hard-coded timeout of 4s for making JS API calls (mqttJSAPITimeout).

Added an MQTT config option to configure this timeout.

Partially fixes #6191

I think the proper fix would require sending an error to the mqtt client that we could not create a subscription. But this helps to at least bump the limit.

Signed-off-by: Ondrej Belusky [email protected]

@zlymeda zlymeda requested a review from a team as a code owner April 24, 2025 19:48
@derekcollison derekcollison requested a review from kozlovic April 24, 2025 19:51
@zlymeda
Copy link
Author

zlymeda commented Apr 24, 2025

the original mqttJSAPITimeout was 4 seconds. however calling it with mqttJSAStreamNames had timeout of 5 seconds.
so I picked the large and used 5 seconds as a default timeout

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

I was going to ask to leave the default timeout to 4sec and keep the timeout as a parameter so that we would pass jsa.timeout most of time except for that specific situations that occur during a transfer of sessions (which is only when upgrading from a certain version to the other). But I guess making the default now 5sec should be fine. Alternatively, we set it back to 4sec and users that would upgrade from an older release where the session transfer session would occur and the 4sec would be too small, then they could explicitly configure to higher value (at least for the upgrade process).

I don't have strong opinion either way, so I think that is fine. The changes requested are around testing. Thanks!

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Would rather have the new "negative" test along with the others, before the "positive" tests. Otherwise, LGTM.

@kozlovic kozlovic changed the title MQTT: allow custom timeout for JS API calls [ADDED] MQTT: allow custom timeout for JS API calls Apr 25, 2025
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

Jetstream API timeouts on MQTT streams
3 participants