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

Add doc about custom http timeout in RabbitMQ Scaler #523

Merged
merged 4 commits into from
Sep 10, 2021
Merged

Add doc about custom http timeout in RabbitMQ Scaler #523

merged 4 commits into from
Sep 10, 2021

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Sep 1, 2021

Signed-off-by: jorturfer [email protected]

This PR adds the docs related with "Add support to specify custom http timeout only for RabbitMQ Scaler"

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Fixes kedacore/keda#2053

Signed-off-by: jorturfer <[email protected]>
@@ -39,7 +39,8 @@ triggers:
- `queueLength`: DEPRECATED! Use `mode: QueueLength` and `value: ##` instead. Target value for queue length passed to the scaler. Example: if one pod can handle 10 messages, set the queue length target to 10. If the actual number of messages in the queue is 30, the scaler scales to 3 pods. Default is 20 unless `publishRate` is specified, in which case `queueLength` is disabled for this trigger.
- `useRegex`: In case of `http` protocol, this parameter allows to use regex (in `queueName` parameter) to select queue instead of full name, the valid values are: `"true"` and `"false"`. Optional.
- `operation`: Operation that will be applied to compute the number of messages in case of `useRegex` enabled. Either `sum` (default),`max`, or `avg`. Optional.
- `metricName` - an optional name to assign to the metric. If not set KEDA will generate a name based on the queue name. If using more than one trigger it is required that all metricNames be unique.
- `metricName`: an optional name to assign to the metric. If not set KEDA will generate a name based on the queue name. If using more than one trigger it is required that all metricNames be unique.
- `timeout`: Timeout **for the specific trigger** in case of `http` protocol. This value will override the value defined in `KEDA_HTTP_DEFAULT_TIMEOUT`. Optional.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean with "for the specific trigger"?

Copy link
Member Author

@JorTurFer JorTurFer Sep 2, 2021

Choose a reason for hiding this comment

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

That this timeout only for this trigger, and it will not affect to other triggers. You can have 2 different triggers that uses RabbitMQ with totally different timeouts. (Maybe is my translation a "false friend"? :( )

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use for this specific trigger?

Copy link
Member

Choose a reason for hiding this comment

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

That, or change the docs for the "global" setting to emphasize that it's global because the trigger docs typically focus on the trigger itself.

If you know what I mean 😅

Copy link
Member Author

@JorTurFer JorTurFer Sep 2, 2021

Choose a reason for hiding this comment

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

I got it, I can do both :)
I have already updated to change from for the specific trigger to for this specific trigger, but I can change also this "This value will override the value defined in KEDA_HTTP_DEFAULT_TIMEOUT" with "This value will override the global value defined in KEDA_HTTP_DEFAULT_TIMEOUT"
Is it better in your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

Where is KEDA_HTTP_DEFAULT_TIMEOUT documented other than the Helm chart? I'd make it explicit there that this is global

Copy link
Member Author

@JorTurFer JorTurFer Sep 2, 2021

Choose a reason for hiding this comment

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

It's already explained in docs

Some scalers issue HTTP requests to external servers (i.e. cloud services). Each applicable scaler uses its own dedicated HTTP client with its own connection pool, and by default each client is set to time out any HTTP request after 3 seconds.
...

That's why I thought that you were saying to clarify it in this page 😅

Copy link
Member

Choose a reason for hiding this comment

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

Good enough, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

okey, then I leave it as it is now :)

Signed-off-by: Jorge Turrado <[email protected]>
@@ -39,7 +39,8 @@ triggers:
- `queueLength`: DEPRECATED! Use `mode: QueueLength` and `value: ##` instead. Target value for queue length passed to the scaler. Example: if one pod can handle 10 messages, set the queue length target to 10. If the actual number of messages in the queue is 30, the scaler scales to 3 pods. Default is 20 unless `publishRate` is specified, in which case `queueLength` is disabled for this trigger.
- `useRegex`: In case of `http` protocol, this parameter allows to use regex (in `queueName` parameter) to select queue instead of full name, the valid values are: `"true"` and `"false"`. Optional.
- `operation`: Operation that will be applied to compute the number of messages in case of `useRegex` enabled. Either `sum` (default),`max`, or `avg`. Optional.
- `metricName` - an optional name to assign to the metric. If not set KEDA will generate a name based on the queue name. If using more than one trigger it is required that all metricNames be unique.
- `metricName`: an optional name to assign to the metric. If not set KEDA will generate a name based on the queue name. If using more than one trigger it is required that all metricNames be unique.
- `timeout`: Timeout **for this specific trigger** in case of `http` protocol. This value will override the value defined in `KEDA_HTTP_DEFAULT_TIMEOUT`. Optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

@JorTurFer does the first sentence mean to say that the timeout only applies for this specific trigger, only if it's the http protocol? If so, here's a suggestion:

Suggested change
- `timeout`: Timeout **for this specific trigger** in case of `http` protocol. This value will override the value defined in `KEDA_HTTP_DEFAULT_TIMEOUT`. Optional.
- `timeout`: Timeout **for this specific trigger**. This value will override the value defined in `KEDA_HTTP_DEFAULT_TIMEOUT`. Optional. Only apples to hosts that use the `http` protocol

Copy link
Member Author

@JorTurFer JorTurFer Sep 9, 2021

Choose a reason for hiding this comment

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

Nice suggestion! Thanks for doing it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem :)

Co-authored-by: Aaron Schlesinger <[email protected]>

Signed-off-by: jorturfer <[email protected]>
@zroubalik zroubalik enabled auto-merge (squash) September 10, 2021 07:31
@zroubalik zroubalik merged commit d9df408 into kedacore:master Sep 10, 2021
@JorTurFer JorTurFer deleted the timeout branch September 13, 2021 18:08
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.

Add support to specify custom http timeout only for RabbitMQ Scaler
4 participants