lokiore opened a new pull request, #2563: URL: https://github.com/apache/phoenix/pull/2563
### What changes were proposed in this pull request? This PR adds a `TransformType.UNKNOWN((byte) -1)` forward-compatibility sentinel and wires the call sites that can observe it to behave defensively. `SystemTransformRecord` reads the persisted transform type from the `SYSTEM.TRANSFORM` row. Today, a serialized transform-type value that this binary does not recognize (because it was written by a newer binary that introduced a new `TransformType` constant) has no safe representation. This PR introduces `UNKNOWN` so the resolution path can map any unrecognized serialized value to it: - `TransformType.fromSerializedValue(...)` returns `UNKNOWN` for any byte that does not correspond to a known constant. `UNKNOWN` is never produced via its own serialized value on the normal write path. Call sites that may observe `UNKNOWN` now skip or fail fast with an operator-readable message rather than proceeding against a record they cannot reason about: - `Transform.doCutover` — skips with a `WARN` and returns, keeping the monitor task alive so other transforms are not starved. - `Transform.doForceCutover` — the `updateTransformRecord(COMPLETED)` + `commit()` step is factored into a package-private `finishForceCutover`, which skips when the type is `UNKNOWN`. Because `doCutover` short-circuits on `UNKNOWN`, marking the record `COMPLETED` would otherwise falsely report success for a cutover that never ran. - `TransformTool.validateTransform` — fails fast with an actionable `IllegalStateException` instructing the operator to upgrade the binary to a version that recognizes the type, or remove the `SYSTEM.TRANSFORM` row, before retrying. - `TransformMonitorTask` — skips the record (returns a `SKIPPED` task result) with an operator-readable message, via package-private `skipIfUnknownTransformType`. ### Why are the changes needed? Without a sentinel, a binary that encounters a transform type written by a newer binary would either throw from an unexpected place or silently mis-handle the row. A new constant added later (the realistic next serialized values are small positive bytes) must degrade safely on older binaries: skip or fail fast with a clear operator message, and in particular never mark a transform `COMPLETED` when no cutover was performed. This keeps mixed-version clusters safe during rollout. ### Does this PR introduce _any_ user-facing change? No. This is an internal forward-compatibility guard. The added `UNKNOWN` constant is not reachable through normal writes; on current binaries no `SYSTEM.TRANSFORM` row resolves to it, so existing behavior is unchanged. ### How was this patch tested? New unit tests, all green (19 tests, 0 failures, 0 errors, 0 skipped across the six classes): - `TransformTypeTest` — enum resolution: unrecognized serialized values resolve to `UNKNOWN`; round-trip of known values is preserved; `UNKNOWN` is not produced via its own serialized value. - `TransformDoCutoverUnknownTypeTest` — `doCutover` short-circuits and performs no work on the connection for an `UNKNOWN` record. - `TransformForceCutoverUnknownTypeTest` — `finishForceCutover` does not mark `COMPLETED` and does not commit for an `UNKNOWN` record; a recognized type is still marked `COMPLETED` and committed. - `TransformToolValidateTransformUnknownTypeTest` — `validateTransform` fails fast for an `UNKNOWN` record. - `TransformMonitorTaskUnknownTypeTest` — the monitor returns a `SKIPPED` result for an `UNKNOWN` record and proceeds normally otherwise. `mvn spotless:apply` is clean on the affected modules. #### Known / deferred Two pre-existing low-severity items are intentionally deferred to a follow-up PR to keep this change focused on the sentinel and its guards: 1. `SystemTransformRecord` reads the transform type with `getByte()` on an `INTEGER` column. The currently used and realistic next values are small and safe; a future value `>= 128` could alias under the signed-byte read. The read path should move to `getInt()`. 2. The monitor task's skip-on-`UNKNOWN` path logs a `WARN` on every task-scan cycle while such a record persists, which can be noisy. This should be rate-limited or logged once per record. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8 (1M context)) -- 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]
