set retry time interval configurable, increase the http client read timeout#6320
Conversation
Zhangxunmt
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
graytaylor0,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
Zhangxunmt
changed the title
| public static final int DEFAULT_RETRY_INTERVAL = 60; // default retry interval is 1 minute | ||
| private static final int MIN_RETRY_INTERVAL = 3; | ||
| public static final int MAX_RETRY_INTERVAL = 300; |
There was a problem hiding this comment.
Nit: suggest specifying the time unit in the name, eg. DEFAULT_RETRY_INTERVAL_SECONDS, etc.
|
|
||
| @JsonPropertyDescription("The retry interval for the throttled records. Default is 60s.") | ||
| @JsonProperty(value = "retry_interval_seconds", defaultValue = "" + DEFAULT_RETRY_INTERVAL) | ||
| private int retry_interval_seconds = DEFAULT_RETRY_INTERVAL; |
There was a problem hiding this comment.
Use camelCase here: retryIntervalSeconds
| if (retry_interval_seconds < MIN_RETRY_INTERVAL || retry_interval_seconds > MAX_RETRY_INTERVAL) { | ||
| throw new IllegalArgumentException(String.format("retry interval for throttled records of %d seconds is not valid, valid range is %d - %d", retry_interval_seconds, MIN_RETRY_INTERVAL, MAX_RETRY_INTERVAL)); | ||
| } |
There was a problem hiding this comment.
See how validation is done here: https://github.com/opensearch-project/data-prepper/blob/main/data-prepper-plugins/mutate-event-processors/src/main/java/org/opensearch/dataprepper/plugins/processor/mutateevent/AddEntryProcessorConfig.java#L217-L220
Recommend doing the same here.
There was a problem hiding this comment.
I will use the hibernate.validator as the validation per taylor's comment. I think it's fine to just use Durations.
@DurationMin(seconds = 3)
@DurationMax(seconds = 300)
|
|
||
| @JsonPropertyDescription("The retry interval for the throttled records. Default is 60s.") | ||
| @JsonProperty(value = "retry_interval_seconds", defaultValue = "" + DEFAULT_RETRY_INTERVAL) | ||
| private int retry_interval_seconds = DEFAULT_RETRY_INTERVAL; |
There was a problem hiding this comment.
I would use Duration type for this and rename to retry_interval. With duration type users can either put an ISO_8601 time ("PT3M") or a simple duration time ("60s"). You can validate it with annotations (ex:
There was a problem hiding this comment.
makes sense to me. Initially I think using Duration is an overkill since we don't need a big interval anyways (probably 5 mins is the biggest). But to make it consistent with retryTimeWindow as a Duration too, I think changing it to Duration is fine.
| return; | ||
| } | ||
|
|
||
| LOG.info("Processing {} throttled records ({}s since last retry)", |
There was a problem hiding this comment.
Could this potentially be a NOISY log? If so you should mark it as NOISY
There was a problem hiding this comment.
Yes it's Ok to make it Noisy. I thought this is only printed every x minutes so probably not that frequent. I will mark it as NOISY in the next revision since it still repeats every 60s in busy conditions.
…d timeout Signed-off-by: Xun Zhang <xunzh@amazon.com>
Signed-off-by: Xun Zhang <xunzh@amazon.com>
…imeout (opensearch-project#6320) * set retry time interval configurable and increase the http client read timeout Signed-off-by: Xun Zhang <xunzh@amazon.com> * address comments Signed-off-by: Xun Zhang <xunzh@amazon.com> --------- Signed-off-by: Xun Zhang <xunzh@amazon.com>
…imeout (opensearch-project#6320) * set retry time interval configurable and increase the http client read timeout Signed-off-by: Xun Zhang <xunzh@amazon.com> * address comments Signed-off-by: Xun Zhang <xunzh@amazon.com> --------- Signed-off-by: Xun Zhang <xunzh@amazon.com> Signed-off-by: Nathan Wand <wandna@amazon.com>
3 participants
Description
This PR increased the http client read timeout from 3 to 30 seconds, and implemented a configurable retry time interval with the default value of 60 seconds to reduce the TPS sent to the remote AI server. Since Bedrock throttles request based on requests per minutes, so 60 seconds should be a good default value.
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.