You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@phoenix.apache.org by "lokiore (via GitHub)" <gi...@apache.org> on 2026/06/30 18:55:31 UTC

[PR] PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType [phoenix]

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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType [phoenix]

Posted by "tkhurana (via GitHub)" <gi...@apache.org>.

tkhurana commented on code in PR #2563:
URL: https://github.com/apache/phoenix/pull/2563#discussion_r3582043241


##########
phoenix-core-client/src/main/java/org/apache/phoenix/schema/PTable.java:
##########


Review Comment:
   Make the "UNKNOWN is never persisted" invariant self-enforcing. Right now it's only convention — getSerializedValue() returns -1 for UNKNOWN, and nothing stops it being
     written; it's safe today only because validateTransform's upstream fail-fast happens to gate every write path (TransformClient.java:373 upsert, Transform.java:129
     delete). Fix: throw in getSerializedValue() when this == UNKNOWN, so any persist attempt fails loudly at the type boundary regardless of call path. This subsumes the
     need for separate guards on removeTransformRecord/completeTransform/abortTransform. Plus a test that the accessor throws for UNKNOWN and known types still round-trip.



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType [phoenix]

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.

virajjasani commented on PR #2563:
URL: https://github.com/apache/phoenix/pull/2563#issuecomment-4847274349

   https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-2563/


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType [phoenix]

Posted by "lokiore (via GitHub)" <gi...@apache.org>.

lokiore merged PR #2563:
URL: https://github.com/apache/phoenix/pull/2563


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType [phoenix]

Posted by "lokiore (via GitHub)" <gi...@apache.org>.

lokiore commented on code in PR #2563:
URL: https://github.com/apache/phoenix/pull/2563#discussion_r3615722943

##########
phoenix-core-client/src/main/java/org/apache/phoenix/schema/PTable.java:
##########

Review Comment:
Good call — done in `77060fca3a`. `getSerializedValue()` now throws `IllegalStateException` on `UNKNOWN`, so any persist attempt fails loudly at the type boundary regardless of call path, instead of silently serializing `-1`. The message names the constant for log/stack clarity. Added a test that the accessor throws for `UNKNOWN`, and reworked the round-trip test to loop over `values()` so every known type is covered automatically as new ones are added.

One deliberate divergence from the suggestion: I kept the existing per-site guards (`doCutover`, `finishForceCutover`, `TransformMonitorTask.skipIfUnknownTransformType`, `validateTransform`) rather than treating the throw as subsuming them. Those aren't persist-time checks — they're *graceful-degradation* gates that skip-with-warn (or fail-fast with an operator message) so an UNKNOWN record doesn't starve other transforms or get falsely marked COMPLETED. They compare `== UNKNOWN` and never reach the accessor, so the throw is a complementary backstop at the persist boundary rather than a replacement. Happy to reconsider if you'd prefer consolidating any of them.

FYI @tkhurana

_(Posted by Loki's Claude companion)_

--
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org