RocMarshal commented on code in PR #10:
URL:
https://github.com/apache/flink-connector-http/pull/10#discussion_r2593047048
##########
docs/content.zh/docs/connectors/table/http.md:
##########
@@ -241,6 +241,25 @@ POST, PUT and GET operations. This query creator allows
you to issue json reques
your own custom http connector. The mappings from columns to the json request
are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and
`http.request.url-map`.
+### Format considerations
+
+#### For http requests
+In order to use custom format, user has to specify option
`'lookup-request.format' = 'customFormatName'`, where `customFormatName` is the
identifier of custom format factory.
Review Comment:
```suggestion
In order to use custom format, users have to specify the option
`'lookup-request.format' = 'customFormatName'`, where `customFormatName` is the
identifier of custom format factory.
```
##########
flink-connector-http/src/test/java/org/apache/flink/connector/http/table/lookup/HttpLookupTableSourceITCaseTest.java:
##########
@@ -1123,6 +1127,35 @@ private void
assertEnrichedRowsNoDataBadStatus(Collection<Row> collectedRows) {
});
}
+ private void assertEnrichedRowsNoDataGoodStatus(Collection<Row>
collectedRows) {
+
+ final int rowArity = 10;
+ // validate every row and its column.
+
+ assertAll(
Review Comment:
I noticed that the API style in the assertion sections of the Flink HTTP
test cases is quite arbitrary, with a mix of Jupiter Assertions and AssertJ
Assertions being used.
I’d like to briefly share two small suggestions:
- A: Wouldn’t it be better to align with the assertion style used in the
Flink repository? That is, to use only AssertJ-related assertion APIs to
implement assertion statements.
- B: Building on A, we could handle the existing assertion statements as
follows:
- a) There’s no need to rush and change them to the AssertJ style, as this
is a historical issue and not changing it won’t affect the assertion
functionality. It would be a good approach to restart the API migration for
this part when our contributors have enough bandwidth to focus on it.
- b) For newly introduced test case code, we expect to use AssertJ
assertion APIs to complete the assertion statements.
--
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]
