HTTP lookup source max retries #90
Conversation
| @@ -27,6 +27,10 @@ | |||
| @Slf4j | |||
| public class JavaNetHttpPollingClient implements PollingClient<RowData> { | |||
|
|
|||
| public static final String DEFAULT_REQUEST_MAX_RETRIES = "3"; | |||
|
|
|||
| public static final String DEFAULT_REQUEST_RETRY_TIMEOUT_MS = "1000"; | |||
There was a problem hiding this comment.
I suggest calling this DEFAULT_REQUEST_RETRY_INTERVAL_MS as it is not a timeout. Also this name should mention lookup or polling client - so that it is obvious it does not apply to the sink client.
Same naming consideration for max retries.
I assume we will add something similar for the sink client
There was a problem hiding this comment.
@Raz0r could you apply proposed changes?
| @@ -27,6 +27,10 @@ | |||
| @Slf4j | |||
| public class JavaNetHttpPollingClient implements PollingClient<RowData> { | |||
|
|
|||
| public static final String DEFAULT_REQUEST_MAX_RETRIES = "3"; | |||
There was a problem hiding this comment.
should this not be an int?
There was a problem hiding this comment.
We tend keep all properties as Strings, becase they are passed as Properties<String,String> from Table factories.
Later we use them like this:
this.httpRequestMaxRetries = Integer.parseInt(
options.getProperties().getProperty(
HttpConnectorConfigConstants.LOOKUP_HTTP_MAX_RETRIES,
DEFAULT_REQUEST_MAX_RETRIES
)
);
| @@ -74,15 +96,43 @@ public Optional<RowData> pull(RowData lookupRow) { | |||
| } | |||
| } | |||
|
|
|||
| // TODO Add Retry Policy And configure TimeOut from properties | |||
There was a problem hiding this comment.
I think the intent of this comment is to make use of the Flink Retry strategies, which includes async.
There was a problem hiding this comment.
The thing is that AsyncSink uses totally different, dedicated mechanism for retries => Flink native support.
And actually there is separate TODO for this in HttpSinkWriter.
What kind of Flink retry strategies you were here @davidradl ?
There was a problem hiding this comment.
I have made some comments - I think we should have consistent retry mechanisms for sink sourcs async synch. There are Retry strategies in Flink e.g. AsyncRetryStrategies. I would hope we could make an IOException bubble up so that the retry strategies would retry .
In Flink I see existing similar options to those you are suggesting:
@JsonTypeName("RetryLookupOptions")
public static class RetryLookupOptions {
public static final String FIELD_NAME_RETRY_PREDICATE = "retry-predicate";
public static final String FIELD_NAME_RETRY_STRATEGY = "retry-strategy";
public static final String FIELD_NAME_RETRY_FIXED_DELAY = "fixed-delay";
public static final String FIELD_NAME_RETRY_MAX_ATTEMPTS = "max-attempts";
|
Hey @davidradl thanks, didn't know about existing |
Description
Lookup queries are now retried in case of IOException up to
gid.connector.http.source.lookup.max-retrieswith a delay ofgid.connector.http.source.lookup.request.retry-timeout-msbetween retries. The default values are 3 retries and 1 second delay.Resolves #66
PR Checklist