github-actions[bot] commented on code in PR #63490:
URL: https://github.com/apache/doris/pull/63490#discussion_r3286821445


##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/insert/streaming/DataSourceConfigValidator.java:
##########
@@ -118,6 +119,45 @@ public static void validateSource(Map<String, String> 
input,
                     "ssl_mode '" + DataSourceConfigKeys.SSL_MODE_VERIFY_CA
                             + "' requires ssl_rootcert to be set");
         }
+
+        validateServerIdConfig(input);
+    }
+
+    // Shared by validateSource and the cdc_stream TVF entrypoint so both 
reject malformed
+    // server_id at SQL-analysis time, not as a cdc_client runtime error.
+    public static void validateServerIdConfig(Map<String, String> input)
+            throws IllegalArgumentException {
+        String serverIdValue = input.get(DataSourceConfigKeys.SERVER_ID);
+        if (serverIdValue == null) {
+            return;
+        }
+        int[] range = parseServerIdRange(serverIdValue);
+        if (range == null) {
+            throw new IllegalArgumentException(
+                    "Invalid value for key '" + DataSourceConfigKeys.SERVER_ID 
+ "': "
+                            + serverIdValue
+                            + ". Expected a single value (e.g. '5400') or 
range (e.g. '5400-5408')"
+                            + " with start >= 1 and start <= end.");
+        }
+        String parallelismValue = input.getOrDefault(
+                DataSourceConfigKeys.SNAPSHOT_PARALLELISM,

Review Comment:
   This cross-field check is correct for CREATE, but the ALTER JOB path 
currently calls `validateSource(this.getSourceProperties(), ...)` with only the 
delta map before `StreamingInsertJob.alterJob()` merges it into the existing 
source properties. That means an existing MySQL job created with 
`"snapshot_parallelism" = "4"` can be altered with only `"server_id" = 
"99001"`; this code defaults the missing parallelism to 1 and accepts it, but 
the effective runtime config after merge is width 1 with parallelism 4, so 
later snapshot readers will hit the same invalid configuration this PR is 
trying to reject at analysis time. Please validate the merged source properties 
in the ALTER path, or reject changing `server_id` for existing jobs.



-- 
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]

Reply via email to