Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
boring-cyborg[bot] commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1940629810 Awesome work, congrats on your first merged pull request! -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
MartijnVisser merged PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
mas-chen commented on code in PR #84:
URL:
https://github.com/apache/flink-connector-kafka/pull/84#discussion_r1487301708
##
flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaTableTestUtils.java:
##
@@ -124,4 +128,12 @@ public static void comparedWithKeyAndOrder(
matching(TableTestMatchers.deepEqualTo(expectedData.get(key), false)));
}
}
+
+private static String rowToString(Object o) {
+if (o instanceof Row) {
Review Comment:
This confused me but I understand it is to provide compatibility for 1.19,
I'd leave a comment since that's not clear
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
MartijnVisser commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1938866968 @mas-chen Any more considerations from your end, else I'm inclined to approve and merge this. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
Jiabao-Sun commented on code in PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#discussion_r1483154745 ## flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/testutils/YamlFileMetadataService.java: ## @@ -23,20 +23,19 @@ import org.apache.flink.connector.kafka.dynamic.metadata.KafkaMetadataService; import org.apache.flink.connector.kafka.dynamic.metadata.KafkaStream; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.DumperOptions; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.TypeDescription; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.Yaml; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.constructor.Constructor; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.nodes.Node; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.nodes.SequenceNode; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.nodes.Tag; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.representer.Representer; - import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableMap; import org.apache.kafka.clients.CommonClientConfigs; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.yaml.snakeyaml.DumperOptions; +import org.yaml.snakeyaml.TypeDescription; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.Constructor; +import org.yaml.snakeyaml.nodes.Node; +import org.yaml.snakeyaml.nodes.SequenceNode; +import org.yaml.snakeyaml.nodes.Tag; +import org.yaml.snakeyaml.representer.Representer; Review Comment: Thanks @MartijnVisser. I have split it into two commits. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
MartijnVisser commented on code in PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#discussion_r1483132533 ## flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/testutils/YamlFileMetadataService.java: ## @@ -23,20 +23,19 @@ import org.apache.flink.connector.kafka.dynamic.metadata.KafkaMetadataService; import org.apache.flink.connector.kafka.dynamic.metadata.KafkaStream; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.DumperOptions; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.TypeDescription; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.Yaml; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.constructor.Constructor; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.nodes.Node; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.nodes.SequenceNode; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.nodes.Tag; -import org.apache.flink.shaded.jackson2.org.yaml.snakeyaml.representer.Representer; - import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableMap; import org.apache.kafka.clients.CommonClientConfigs; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.yaml.snakeyaml.DumperOptions; +import org.yaml.snakeyaml.TypeDescription; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.Constructor; +import org.yaml.snakeyaml.nodes.Node; +import org.yaml.snakeyaml.nodes.SequenceNode; +import org.yaml.snakeyaml.nodes.Tag; +import org.yaml.snakeyaml.representer.Representer; Review Comment: Nit: it's good to see this addressed (it has a separate ticket at https://issues.apache.org/jira/browse/FLINK-34193), we should have changes like these in a separate commit to make reviews easier. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
pvary commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1934089389 The SinkV2 related changes are ok (There is some extra casts which are needed) -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
Jiabao-Sun commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1933308568 By the way, this PR was blocked by https://github.com/apache/flink/pull/24249 before is that InitContextWrapper does not implement the metadataConsumer (introduced by FLINK-25696) method, but we use it in KafkaSinkITCase, which causes the behavior of metadataConsumer to not take effect. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
Jiabao-Sun commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1933305498 Thanks @mas-chen for the review. The main reason for this PR is in version 1.19, due to the introduction of WriterInitContext by FLINK-33973. In terms of code changes, KafkaSink.createWriter is used to ensure that TestSinkInitContext can correctly create KafkaWriter regardless of whether it inherits from WriterInitContext or Sink.InitContext. https://github.com/apache/flink/blob/1b95b191922829fd8e7a76e5c9d8de68bb57ae7d/flink-core/src/main/java/org/apache/flink/api/connector/sink2/Sink.java#L66-L78 https://github.com/apache/flink/pull/24180/files#r1467200868 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
mas-chen commented on code in PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#discussion_r1482311723 ## .github/workflows/push_pr.yml: ## @@ -30,6 +30,8 @@ jobs: include: - flink: 1.18.1 jdk: '8, 11, 17' + - flink: 1.19-SNAPSHOT Review Comment: Are the rest of the changes due to removed internal APIs from Flink (e.g. the metrics stuff)? I would rename this PR as "Add build option for 1.19 to verify SinkV2 backward compatibility" or similar -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
mas-chen commented on code in PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#discussion_r1482310252 ## .github/workflows/push_pr.yml: ## @@ -30,6 +30,8 @@ jobs: include: - flink: 1.18.1 jdk: '8, 11, 17' + - flink: 1.19-SNAPSHOT Review Comment: Is this the only relevant thing in this PR? Everything else looks like refactoring... After https://github.com/apache/flink/pull/24249 was merged, this compiles which confirms no breaking changes? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
MartijnVisser commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1932188194 @pvary @mas-chen Can either of you want to take a look at this PR? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
Jiabao-Sun commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1931934622 CI passed locally https://github.com/Jiabao-Sun/flink-connector-kafka/actions/runs/7814231077. @MartijnVisser, could you help review it? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
Jiabao-Sun commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1931860747 > @Jiabao-Sun Will you check if this PR can now be completed? ok -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34192] Update to be compatible with updated SinkV2 interfaces [flink-connector-kafka]
MartijnVisser commented on PR #84: URL: https://github.com/apache/flink-connector-kafka/pull/84#issuecomment-1931721756 @Jiabao-Sun Will you check if this PR can now be completed? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
