ferenc-csaky commented on code in PR #6:
URL:
https://github.com/apache/flink-connector-http/pull/6#discussion_r2515434813
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/HttpLookupConnectorOptions.java:
##########
@@ -74,6 +75,18 @@ public class HttpLookupConnectorOptions {
public static final ConfigOption<String> LOOKUP_QUERY_CREATOR_IDENTIFIER =
ConfigOptions.key(SOURCE_LOOKUP_QUERY_CREATOR_IDENTIFIER).stringType().noDefaultValue();
+ public static final ConfigOption<String> LOOKUP_HTTP_VERSION =
+ ConfigOptions.key(SOURCE_LOOKUP_QUERY_HTTP_VERSION)
+ .stringType()
+ .noDefaultValue()
+ .withDescription(
Review Comment:
I think this description would benefit using the `DescriptionBuilder` and
emphasizing the possible values in a list. For example how `execution.target`
desc is constructed [in the core Flink
repo](https://github.com/apache/flink/blob/11af4bf01b8836fd2e51a429a22310fb433cc113/flink-core/src/main/java/org/apache/flink/configuration/DeploymentOptions.java#L45).
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/RequestFactoryBase.java:
##########
@@ -102,12 +114,20 @@ public HttpLookupSourceRequestEntry
buildLookupRequest(RowData lookupRow) {
protected abstract Logger getLogger();
/**
- * Method for preparing {@link Builder} for concrete REST method.
+ * Method for preparing {@link Builder} for REST method.
*
* @param lookupQuery lookup query used for request query parameters or
body.
* @return {@link Builder} for given lookupQuery.
*/
- protected abstract Builder setUpRequestMethod(LookupQueryInfo lookupQuery);
+ protected Builder setUpRequestMethod(LookupQueryInfo lookupQuery) {
+ HttpRequest.Builder builder =
+ HttpRequest.newBuilder()
+
.timeout(Duration.ofSeconds(this.httpRequestTimeOutSeconds));
+ if (httpVersion != null) {
+ builder.version(httpVersion);
+ }
+ return builder;
Review Comment:
nit: maybe simplify to `return httpVersion == null ? builder :
builder.version(httpVersion)`
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/HttpLookupConnectorOptions.java:
##########
@@ -74,6 +75,18 @@ public class HttpLookupConnectorOptions {
public static final ConfigOption<String> LOOKUP_QUERY_CREATOR_IDENTIFIER =
ConfigOptions.key(SOURCE_LOOKUP_QUERY_CREATOR_IDENTIFIER).stringType().noDefaultValue();
+ public static final ConfigOption<String> LOOKUP_HTTP_VERSION =
+ ConfigOptions.key(SOURCE_LOOKUP_QUERY_HTTP_VERSION)
+ .stringType()
+ .noDefaultValue()
+ .withDescription(
+ "Version of HTTP to use for lookup HTTP requests. "
+ + "The valid values are HTTP_1_1 and
HTTP_2, which specify HTTP 1.1 or 2"
+ + " respectively. This option may be
required as HTTP_1_1, if the"
+ + " endpoint is http 1.1, because some
http 1.1 endpoints reject HTTP"
Review Comment:
nit: http -> HTTP (2x)
--
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]
