Configure retry on Rate-Limiting from remote-write config #8477
Conversation
Harkishen-Singh
force-pushed
the
configure-rate-limit-via-config
branch
from
90a9f97 to
72d031e
Compare
docs/configuration/configuration.md
Outdated
| @@ -1724,6 +1724,9 @@ url: <string> | |||
| headers: | |||
| [ <string>: <string> ... ] | |||
|
|
|||
| # Retry on receiving a rate-limiting status code in response from remote-write storage. | |||
There was a problem hiding this comment.
Would you put a note here saying that this configuration may change in any release, similar to the note above metadata_config. I imagine we may get rid of it once there is an agreed upon solution to how to handle rate limited retries.
There was a problem hiding this comment.
HTTP 429 should be mentioned.
I am not confortable to change this behavior in a minor release.
There was a problem hiding this comment.
This doc string should also say retry on 429 status code, and the variable name two lines down still needs to be updated.
There was a problem hiding this comment.
variable name?
There was a problem hiding this comment.
retry_on_rate_limit should be retry_on_http_429 now a couple lines below this.
There was a problem hiding this comment.
Done, I hope the message is good.
docs/configuration/configuration.md
Outdated
| @@ -1724,6 +1724,9 @@ url: <string> | |||
| headers: | |||
| [ <string>: <string> ... ] | |||
|
|
|||
| # Retry on receiving a rate-limiting status code in response from remote-write storage. | |||
There was a problem hiding this comment.
HTTP 429 should be mentioned.
I am not confortable to change this behavior in a minor release.
Harkishen-Singh
force-pushed
the
configure-rate-limit-via-config
branch
from
72d031e to
653e37d
Compare
Harkishen-Singh
force-pushed
the
configure-rate-limit-via-config
branch
from
653e37d to
4a175dd
Compare
Harkishen-Singh
force-pushed
the
configure-rate-limit-via-config
branch
from
4a175dd to
a7bc54d
Compare
|
Let's hold off a bit on this since #8237 will be reverted for the release while we figure out what to do about limiting retries. |
|
The release branch has been merged back into master, so we can add this flag without worrying about merge conflicts now once the final comments are addressed. |
…nfig. Signed-off-by: Harkishen-Singh <[email protected]>
Harkishen-Singh
force-pushed
the
configure-rate-limit-via-config
branch
from
a7bc54d to
77c20fd
Compare
|
When I see this, I think it is a mistake to put that in the queue config because it is actually managed by the client. Sorry I did not spot this before. Even if it looks better next to backoff, it is actually really a client thing. Maybe we need to revert to having it at the client level again. so sorry about this. cc @csmarchbanks what is your opinion here? |
|
That's completely fine. No problem. I am happy to revert back if Chris agrees too. |
There was a problem hiding this comment.
I slightly prefer having it in the queue_config. The client happens to be where the implementation uses the flag (right now at least), but it is the queue that actually does the retries. I can imagine a future where there is a RateLimitedError in addition to RecoverableError at which point the flag would likely be used by the queue. Not a strong opinion and I would be fine having the toggle near the client config as well.
Approving as i am happy with this as stands, @roidelapluie if you agree with my slight preference feel free to merge, otherwise let's go back to what it was before.
|
Okay, I agree from a user perspective, it makes more sense in the queue config. Thanks @Harkishen-Singh !!! |
Signed-off-by: Harkishen-Singh [email protected]
Fixes: #8474
cc @csmarchbanks @roidelapluie