Copilot commented on code in PR #64310:
URL: https://github.com/apache/doris/pull/64310#discussion_r3385120449
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/insert/streaming/StreamingInsertJob.java:
##########
@@ -709,12 +709,12 @@ protected void fetchMeta() throws JobException {
||
!InternalErrorCode.MANUAL_PAUSE_ERR.equals(this.getFailureReason().getCode())) {
// When a job is manually paused, it does not need to be set
again,
// otherwise, it may be woken up by auto resume.
+ // Pause before setting the reason: updateJobStatus's
writeLock orders this after any
+ // task-success callback that clears failureReason, so a
success can't wipe the reason.
+ this.updateJobStatus(JobStatus.PAUSED);
this.setFailureReason(
new
FailureReason(InternalErrorCode.GET_REMOTE_DATA_ERROR,
Review Comment:
The new ordering (pause then setFailureReason) still doesn’t guarantee the
reason won’t be cleared: `failureReason` is mutated via Lombok’s
`setFailureReason()` without taking the job’s `lock`, and
`onStreamTaskSuccess()`/`resetFailureInfo(null)` clear it without acquiring
`writeLock()`. This makes the added comment about `updateJobStatus`’s writeLock
ordering misleading, and the original race (PAUSED with empty reason) can still
occur.
To make this reliable, updates/clears of `failureReason` should be
consistently guarded by the same job write lock (e.g., wrap
`resetFailureInfo()`/`onStreamTaskSuccess()` and the setters used by the
scheduler in `writeLock()/writeUnlock()`, or replace Lombok’s setter with a
lock-protected implementation).
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/insert/streaming/PostgresResourceValidator.java:
##########
@@ -123,6 +127,17 @@ private static String resolvePublicationName(Map<String,
String> config, String
return StringUtils.isNotBlank(name) ? name :
DataSourceConfigKeys.defaultPublicationName(jobId);
}
+ private static void checkDatabaseNameLength(String database) throws
JobException {
+ if (StringUtils.isBlank(database)) {
+ return;
+ }
+ if (database.length() > Config.streaming_pg_max_identifier_length) {
+ throw new JobException("database name '" + database + "' exceeds
max length "
+ + Config.streaming_pg_max_identifier_length + ";
PostgreSQL truncates it and"
+ + " the replication-slot lookup would fail.");
+ }
+ }
Review Comment:
PostgreSQL’s identifier length limit is defined in bytes (NAMEDATALEN-1),
but this check uses `database.length()` (UTF-16 code units), which can
undercount for non-ASCII names and still allow names that PostgreSQL will
truncate. Consider checking UTF-8 byte length instead and clarifying the error
message accordingly.
--
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]
