njnu-seafish commented on PR #18549:
URL:
https://github.com/apache/dolphinscheduler/pull/18549#issuecomment-5291651575
> ### Preserve the existing `sendEmail` field name
> `SqlParameters` renames the persisted/API field from `sendEmail` to
`sendAlert`, and the UI now only reads/writes `sendAlert`.
>
> Although `@JsonAlias("sendEmail")` keeps deserialization compatible,
serialization and UI payloads use the new name. This can cause existing
clients, SDKs, integrations, or mixed-version components to silently lose the
setting. It also introduces an unnecessary database migration and an
incompatible public contract change.
>
> Please keep `sendEmail` as the field name and only update the semantic
description/UI label to indicate that alerts can use channels other than email.
If the rename is still required, please provide an explicit compatibility
strategy covering API clients, UI loading of legacy definitions, and rolling
upgrades.
>
> ### Keep `AlertSendRequest` wire-compatible
> `AlertSendRequest` changes `warnType: int` to `alertType: AlertType`.
>
> This changes both the field name and the serialized type of the
Master–Alert RPC request. During a rolling upgrade, an old Alert Server will
still expect `warnType`, while a new Alert Server may receive a request without
`alertType` from an old Master. The result can be a default/incorrect alert
type or a `NullPointerException` at `alertType.getCode()`.
>
> Please retain the existing `warnType` field for compatibility, or support
both fields with explicit conversion and add a mixed-version serialization test.
Good point, your consideration is very thorough. I didn't take the rolling
upgrade scenario into account. I have reverted the breaking changes.
--
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]
