bharathgunapati commented on PR #34:
URL:
https://github.com/apache/flink-connector-http/pull/34#issuecomment-6470691620
Hi @featzhang, a default User-Agent is a nice improvement. A few things
would get this over the line:
1. A rebase: `JavaNetSinkHttpClient` now takes an `HttpSinkConfig` after
#54, and the new option needs the same `fromDataStream` mapping in
`HttpSinkConfigFactory` as the other sink options, otherwise
`setProperty("http.user-agent", ...)` is ignored for DataStream users.
2. The precedence point David raised: because the option has a default, the
conflict check currently throws for every job that sets
`http.sink.header.User-Agent` today, which would break existing configurations.
Rejecting only when the option was set explicitly, and letting the header win
otherwise, would fix it.
3. The conflict check compares header names case-sensitively
(`containsKey("User-Agent")`), so `http.sink.header.user-agent` slips through
and the request goes out with two User-Agent headers. Header names should be
compared ignoring case, as the OIDC code does for `Authorization`.
4. `RequestFactoryBase` calls `options.getReadableConfig().get(...)` without
a null guard; other lookup code checks that config for null, so either guard it
or show it cannot be null there.
5. The check runs when the writer or lookup function opens, not at table
creation as the docs say. Either validate in the factories so a bad DDL fails
at planning, or adjust the wording.
A small naming request too: `http.user-agent` would match the kebab-case
convention we moved to with FLINK-40149.
cc: @davidradl
--
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]
