Copilot commented on code in PR #692:
URL:
https://github.com/apache/doris-flink-connector/pull/692#discussion_r3869196613
##########
flink-doris-connector/flink-doris-connector-flink1/src/test/java/org/apache/doris/flink/source/DorisSourceOptionsTest.java:
##########
@@ -190,16 +190,21 @@ void validatesOffsetPersistenceOptions() {
.setBinlogConsumerId("prod.sales.orders")
.build()))
.hasMessageContaining("jdbc-url");
- assertThat(
- buildSource(
- DorisReadOptions.builder()
-
.setScanMode(DorisSourceScanMode.LATEST)
-
.setBinlogOffsetTable("ops.flink_source_offsets")
-
.setBinlogConsumerId("prod.sales.orders")
- .build(),
- "db.table",
- "jdbc:mysql://127.0.0.1:9030"))
- .isNotNull();
+ }
+
+ @Test
+ void validatesOffsetTableBeforeCreatingSource() {
+ assertThatThrownBy(
+ () ->
+ buildSource(
+ DorisReadOptions.builder()
+
.setScanMode(DorisSourceScanMode.LATEST)
+
.setBinlogOffsetTable("ops.flink_source_offsets")
+
.setBinlogConsumerId("prod.sales.orders")
+ .build(),
+ "db.table",
+
"jdbc:mysql://127.0.0.1:1?connectTimeout=100"))
Review Comment:
This test makes a real TCP connection attempt to localhost:1. That can be
environment-dependent (e.g., port unexpectedly open/filtered) and can introduce
flakiness. Prefer a JDBC URL that fails deterministically at URL/port
validation time without any network IO.
##########
flink-doris-connector/flink-doris-connector-base/src/main/java/org/apache/doris/flink/source/reader/DorisOffsetPublisher.java:
##########
@@ -36,16 +40,36 @@ public DorisOffsetPublisher(
if (tableParts.length != 2 || tableParts[0].isEmpty() ||
tableParts[1].isEmpty()) {
throw new IllegalArgumentException("Offset table must use
database.table format");
}
+ this.database = tableParts[0];
+ this.table = tableParts[1];
this.insertSql =
"INSERT INTO "
- + quoteIdentifier(tableParts[0])
+ + quoteIdentifier(database)
+ "."
- + quoteIdentifier(tableParts[1])
+ + quoteIdentifier(table)
+ " (`consumer_id`, `offset_timestamp`, `update_time`)
"
+ "VALUES (?, ?, CURRENT_TIMESTAMP(3))";
this.consumerId = consumerId;
}
+ public void validateOffsetTable() {
+ try {
+ Connection connection =
connectionProvider.getOrEstablishConnection();
+ DatabaseMetaData metadata = connection.getMetaData();
+ try (ResultSet tables = metadata.getTables(database, null, table,
null)) {
+ while (tables.next()) {
+ if (table.equals(tables.getString("TABLE_NAME"))) {
+ return;
+ }
+ }
+ }
+ } catch (Exception error) {
+ throw new DorisRuntimeException(
+ "Failed to validate offset table: " + database + "." +
table, error);
+ }
+ throw new DorisRuntimeException("Offset table does not exist: " +
database + "." + table);
+ }
Review Comment:
validateOffsetTable() only checks TABLE_NAME, but does not verify that the
returned row belongs to the exact configured database. Because
DatabaseMetaData#getTables parameters are SQL patterns (where '_' and '%' are
wildcards), a database name containing those characters could match multiple
catalogs and incorrectly validate against a table with the same name in a
different database.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
