featzhang commented on PR #31: URL: https://github.com/apache/flink-connector-http/pull/31#issuecomment-4293171939
You're absolutely right that the runtime capability already exists in `RequestFactoryBase.java:82` — the `httpRequestTimeOutSeconds` field is already used to set the timeout via `HttpRequest.Builder.timeout(Duration.ofSeconds(this.httpRequestTimeOutSeconds))`. However, before this PR, the timeout options were **not exposed as typed `ConfigOption` definitions** in the connector's option classes. This meant: 1. **No validation**: The options weren't listed in `optionalOptions()` of the `DynamicTableFactory` implementations, so Flink couldn't validate them during DDL parsing. 2. **Confusing user experience**: Users who configured `http.source.lookup.request.timeout` or `http.sink.request.timeout` via SQL DDL received "unsupported option" warnings, even though the options worked at runtime. 3. **No type safety**: The timeout had to be parsed from `Properties` as a string, with no guarantee it was a valid duration format. 4. **No discoverability**: No IDE autocomplete, no default value in documentation, and the `TODO` comment in `HttpLookupTableSourceFactory` noted this gap. This PR **formalizes the existing runtime capability** by: - Adding proper `ConfigOption<Duration>` definitions (`SOURCE_LOOKUP_REQUEST_TIMEOUT` and `SINK_REQUEST_TIMEOUT`) - Registering them in `optionalOptions()` so Flink validates them - Providing type safety and clear default values (30s) - Improving documentation with Duration format examples The runtime behavior is **completely unchanged** — this is purely additive configuration formalization. The enhanced documentation you mentioned is indeed part of the value, but the primary benefit is making these options first-class citizens in Flink's configuration system. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
