Re: [PR] [FLINK-35354] Support host mapping in Flink tikv cdc [flink-cdc]
GOODBOY008 merged PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336 -- 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
GOODBOY008 commented on PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336#issuecomment-2216531933 @Mrart Can you rebase the branch? -- 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
Mrart commented on PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336#issuecomment-2148692911 @leonardBang Can you help me review it again？ -- 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
Mrart commented on PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336#issuecomment-2132210507 > > I think we can specific a config prefix pass config to tikv client like cdc source connector pass config to debezium by `debezium.*`. This way will be flexible. @leonardBang @Mrart cc > > tikv.* is configured on tikv config. hostmapping and pd config are similar to the database host configuration, and the current configuration looks more appropriate Thanks for the help review. -- 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
GOODBOY008 commented on PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336#issuecomment-2132201295 As discuss offline with @Mrart , due to ``tikvclient` config `hostMapping` different from others. So the solution in this PR, I think 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
Mrart commented on PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336#issuecomment-2124764340 > I think we can specific a config prefix pass config to tikv client like cdc source connector pass config to debezium by `debezium.*`. This way will be flexible. @leonardBang @Mrart cc tikv.* is configured on tikv config. hostmapping and pd config are similar to the database host configuration, and the current configuration looks more appropriate -- 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
GOODBOY008 commented on PR #3336: URL: https://github.com/apache/flink-cdc/pull/3336#issuecomment-2124044327 I think we can specific a config prefix pass config to tikv client like cdc source connector pass config to debezium by `debezium.*`. This way will be flexible. @leonardBang @Mrart cc -- 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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
Mrart commented on code in PR #3336:
URL: https://github.com/apache/flink-cdc/pull/3336#discussion_r1608399656
##
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-tidb-cdc/src/main/java/org/apache/flink/cdc/connectors/tidb/TDBSourceOptions.java:
##
@@ -82,10 +90,13 @@ private TDBSourceOptions() {}
.withDescription("TiKV GRPC batch scan concurrency");
public static TiConfiguration getTiConfiguration(
-final String pdAddrsStr, final Map options) {
+final String pdAddrsStr, final String hostMapping, final
Map options) {
Review Comment:
hostMapping is an optional variable and returns tiConfiguration.Adding a new
method would increase the complexity of the code. Change to Optional.of(new
UriHostMapping(hostMapping)).ifPresent(tiConf::setHostMapping);
##
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-tidb-cdc/src/main/java/org/apache/flink/cdc/connectors/tidb/TDBSourceOptions.java:
##
@@ -82,10 +90,13 @@ private TDBSourceOptions() {}
.withDescription("TiKV GRPC batch scan concurrency");
public static TiConfiguration getTiConfiguration(
-final String pdAddrsStr, final Map options) {
+final String pdAddrsStr, final String hostMapping, final
Map options) {
final Configuration configuration = Configuration.fromMap(options);
final TiConfiguration tiConf =
TiConfiguration.createDefault(pdAddrsStr);
+if (StringUtils.isNotBlank(hostMapping)) {
Review Comment:
Optional.of(new
UriHostMapping(hostMapping)).ifPresent(tiConf::setHostMapping);
--
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-35354] Support host mapping in Flink tikv cdc [flink-cdc]
leonardBang commented on code in PR #3336:
URL: https://github.com/apache/flink-cdc/pull/3336#discussion_r1608027592
##
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-tidb-cdc/src/main/java/org/apache/flink/cdc/connectors/tidb/TDBSourceOptions.java:
##
@@ -82,10 +90,13 @@ private TDBSourceOptions() {}
.withDescription("TiKV GRPC batch scan concurrency");
public static TiConfiguration getTiConfiguration(
-final String pdAddrsStr, final Map options) {
+final String pdAddrsStr, final String hostMapping, final
Map options) {
Review Comment:
Could we add a new method named `getTiConfiguration(
final String pdAddrsStr, final Map options) {
final String pdAddrsStr, final String hostMapping, final
Map options)` instead of changing the exits one ?
##
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-tidb-cdc/src/main/java/org/apache/flink/cdc/connectors/tidb/TDBSourceOptions.java:
##
@@ -57,6 +59,12 @@ private TDBSourceOptions() {}
.noDefaultValue()
.withDescription("TiKV cluster's PD address");
+public static final ConfigOption HOST_MAPPING =
+ConfigOptions.key("host-mapping")
Review Comment:
Could we also update connector documentation as well ?
##
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-tidb-cdc/src/main/java/org/apache/flink/cdc/connectors/tidb/TDBSourceOptions.java:
##
@@ -82,10 +90,13 @@ private TDBSourceOptions() {}
.withDescription("TiKV GRPC batch scan concurrency");
public static TiConfiguration getTiConfiguration(
-final String pdAddrsStr, final Map options) {
+final String pdAddrsStr, final String hostMapping, final
Map options) {
final Configuration configuration = Configuration.fromMap(options);
final TiConfiguration tiConf =
TiConfiguration.createDefault(pdAddrsStr);
+if (StringUtils.isNotBlank(hostMapping)) {
Review Comment:
Could we use Flink's `Preconditions` check utils instead of introducing
external dependency ?
##
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-tidb-cdc/src/main/java/org/apache/flink/cdc/connectors/tidb/table/TiDBTableSource.java:
##
@@ -70,12 +72,14 @@ public TiDBTableSource(
String database,
String tableName,
String pdAddresses,
+String hostMapping,
Review Comment:
@Nullable
--
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
