njnu-seafish commented on PR #18549: URL: https://github.com/apache/dolphinscheduler/pull/18549#issuecomment-5423416494
> Thanks for the thorough review! Both issues have been addressed. Thanks for the thorough review! Both issues have been addressed. > The previous field-name, ACK ordering, and task-identity issues have been addressed, but I found three remaining problems: > > 1. TASK_RESULT is not compatible with an old Alert Server during rolling upgrades > > File: dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/enums/AlertType.java > > A new Master now persists `alert_type = 8`. An old Alert Server does not have `AlertType.TASK_RESULT`, so MyBatis-Plus maps this unknown value to null. `AlertSender#getAlertData()` subsequently calls `event.getAlertType().getCode()`, causing a NullPointerException. > > The alert remains in `WAIT_EXECUTION` and is not delivered while the old Alert Server is active. This is the database equivalent of the mixed-version RPC compatibility issue that was fixed earlier. > > Please either reuse a value understood by old Alert Servers, add an explicitly safe upgrade strategy/order, or otherwise ensure that mixed-version deployments cannot write an enum value that an active consumer cannot deserialize. A mixed-version persistence test would be useful. > > 2. Preserve the public AbstractTask alert API > > File: dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractTask.java > > This PR removes the protected `needAlert` / `taskAlertInfo` fields and the public `getNeedAlert`, `setNeedAlert`, `getTaskAlertInfo`, and `setTaskAlertInfo` methods. > > `AbstractTask` is part of the task-plugin extension API. Existing third-party task plugins compiled against these members can fail with `NoSuchMethodError` or `NoSuchFieldError` after upgrading, and source-compatible plugins will no longer compile. > > Please retain these members as deprecated compatibility bridges that delegate to `TaskExecutionContext`. The SQL task can use the new context-based implementation without removing the old extension API immediately. > > 3. The idempotent insert is still vulnerable to concurrent duplicates > > File: dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/AlertMapper.xml Around lines 59–79 > > `INSERT ... SELECT ... HAVING count(*) = 0` is still a check-then-insert operation. There is no unique constraint on `(sign, workflow_instance_id, alert_type)`, so two transactions handling the same success event can both observe a count of zero and insert duplicate alerts, especially on PostgreSQL. > > The current tests only execute the inserts sequentially and therefore do not verify actual idempotency under concurrent delivery. > > Please enforce the idempotency key at the database level and use an atomic conflict-handling insert, or provide another cross-database synchronization mechanism. A concurrent integration test should verify that exactly one row is inserted. Thanks for the thorough review! Both issues have been addressed. -- 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]
