bharathgunapati opened a new pull request, #52:
URL: https://github.com/apache/flink-connector-http/pull/52
## What is the purpose of the change
`request.timeout` is declared as a typed `ConfigOption<Duration>` for both
the
lookup source (`SOURCE_LOOKUP_REQUEST_TIMEOUT`) and the sink
(`SINK_REQUEST_TIMEOUT`), defaulting to 30s, and the docs describe it as a
Flink
`Duration`. However, the runtime never read the declared option — it parsed
the
raw config string with `Integer.parseInt(...)`, so any `Duration`-formatted
value
such as `'30s'` threw `NumberFormatException` at query time.
The failure is easy to miss because the `http.*` namespace is skipped by
`FactoryUtil#validateExcept(...)`: `CREATE TABLE` succeeds and the job only
crashes when the first lookup/sink request is built.
This completes FLINK-39364 (#31), which added the `Duration` option
declarations
and docs but left the runtime read path parsing integer seconds.
## Brief change log
- Lookup: `RequestFactoryBase` reads `SOURCE_LOOKUP_REQUEST_TIMEOUT` from
`ReadableConfig` as a `Duration` and applies it directly.
- Sink: `AbstractRequestSubmitter` parses the value with
`TimeUtils.parseDuration(...)`; `PerRequestSubmitter` /
`BatchRequestSubmitter`
use the `Duration` directly.
- Removed the now-unused integer-seconds field and
`DEFAULT_REQUEST_TIMEOUT_SECONDS`
constant; the default comes from the option (`Duration.ofSeconds(30)`).
- Updated the DataStream docs (en + zh) to describe the value as a Flink
`Duration`.
## Compatibility note
Values are now parsed with standard Flink `Duration` semantics, consistent
with
the sibling `connection.timeout` option:
- Unit-suffixed values (`'30s'`, `'1min'`, `'500ms'`) now work (previously
crashed).
- A bare number such as `'30'` is now interpreted as **30 milliseconds**
(Flink's
default duration unit), whereas the old broken code treated `'30'` as 30
seconds.
The only overlap is bare integers: anyone who wrote `'30'` meaning seconds
should
now write `'30s'`. Any unit-suffixed value was previously non-functional (it
threw), so no working configuration silently changes meaning. This is
documented
and captured in the JIRA release note.
## Verifying this change
Automated tests (Flink 2.2.0, JDK 11/17/21, CI green):
-
`HttpLookupTableSourceITCaseTest#testHttpLookupJoinWithDurationRequestTimeout` —
end-to-end lookup join with `'http.source.lookup.request.timeout' = '30s'`;
fails on `main` with `NumberFormatException`, passes here.
- `BodyBasedRequestFactoryTest` / `BatchRequestSubmitterTest` — pin the
`Duration`
parsing semantics (units, default 30s, and the bare-number-is-milliseconds
rule).
- `HttpDynamicTableSinkFactoryTest#requestTimeoutOptionTest` — a
`Duration`-typed
sink timeout is accepted.
Stock reproduction (optional, for reviewers): a single test on unmodified
pre-fix
code is available at
`bharathgunapati/flink-connector-http@demo/FLINK-40072-request-timeout-repro`
(`HttpLookupTableSourceITCaseTest#testHttpLookupJoinWithDurationRequestTimeoutFailsOnPreFixRuntime`),
which asserts the `NumberFormatException` end users hit at query time.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **no**
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: **no**
- The serializers: **no**
- The runtime per-record code paths (performance sensitive): **no**
- Anything that affects deployment or recovery: **no**
- The S3 file system connector: **no**
## Documentation
- Does this pull request introduce a new feature? **no** (bug fix)
- If yes, how is the feature documented? docs updated to reflect the
`Duration` format
--
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]
