PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType#2563
PHOENIX-7906 :- Add UNKNOWN forward-compat sentinel for TransformType#2563lokiore wants to merge 2 commits into
Conversation
Add a TransformType.UNKNOWN((byte) -1) sentinel so a binary that reads a SYSTEM.TRANSFORM row written by a newer binary (one that introduced a new transform type this binary does not recognize) resolves the unknown serialized value to UNKNOWN instead of throwing or silently mis-mapping it. fromSerializedValue returns UNKNOWN for any unrecognized byte; UNKNOWN is never produced through its own serialized value on the normal write path. Callers that can observe UNKNOWN now handle it defensively rather than proceeding against a record they cannot reason about: - Transform.doCutover: skip with an operator-readable WARN and return, so the monitor task stays alive and other transforms are not starved. - Transform.doForceCutover: the COMPLETED update + commit is factored into a package-private finishForceCutover, which skips when the type is UNKNOWN so an unrun cutover is not falsely marked COMPLETED. - TransformTool.validateTransform: fail fast with an actionable message instructing the operator to upgrade the binary or remove the row. - TransformMonitorTask: skip the record (SKIPPED result) with an operator message via package-private skipIfUnknownTransformType. Unit tests cover the enum resolution and each guard site. Generated-by: Claude Code (Opus 4.8 (1M context))
| return byteValue; | ||
| } | ||
|
|
||
| public int getSerializedValue() { |
There was a problem hiding this 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.
There was a problem hiding this 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)
Address review feedback: make the "UNKNOWN is never persisted" invariant self-enforcing rather than convention. TransformType.getSerializedValue() now throws IllegalStateException when called on UNKNOWN, so any attempt to persist the read-only forward-compatibility sentinel fails loudly at the type boundary regardless of call path, instead of silently serializing -1. The message names the constant for log/stack clarity. The read path is unchanged: fromSerializedValue still resolves any unrecognized byte to UNKNOWN without routing through the accessor. The existing skip-with-warn guards (doCutover, finishForceCutover, TransformMonitorTask, validateTransform) are intentionally retained: they degrade gracefully at the processing boundary and are complementary to, not subsumed by, the persist-boundary throw. Tests: assert getSerializedValue() throws for UNKNOWN, and that every known type round-trips through fromSerializedValue(getSerializedValue()) via a loop over values() so future additions are covered automatically. Generated-by: Claude Code (Opus 4.8 (1M context))
3 participants
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.SystemTransformRecordreads the persisted transform type from theSYSTEM.TRANSFORMrow. Today, a serialized transform-type value that this binary does not recognize (because it was written by a newer binary that introduced a newTransformTypeconstant) has no safe representation. This PR introducesUNKNOWNso the resolution path can map any unrecognized serialized value to it:TransformType.fromSerializedValue(...)returnsUNKNOWNfor any byte that does not correspond to a known constant.UNKNOWNis never produced via its own serialized value on the normal write path.Call sites that may observe
UNKNOWNnow skip or fail fast with an operator-readable message rather than proceeding against a record they cannot reason about:Transform.doCutover— skips with aWARNand returns, keeping the monitor task alive so other transforms are not starved.Transform.doForceCutover— theupdateTransformRecord(COMPLETED)+commit()step is factored into a package-privatefinishForceCutover, which skips when the type isUNKNOWN. BecausedoCutovershort-circuits onUNKNOWN, marking the recordCOMPLETEDwould otherwise falsely report success for a cutover that never ran.TransformTool.validateTransform— fails fast with an actionableIllegalStateExceptioninstructing the operator to upgrade the binary to a version that recognizes the type, or remove theSYSTEM.TRANSFORMrow, before retrying.TransformMonitorTask— skips the record (returns aSKIPPEDtask result) with an operator-readable message, via package-privateskipIfUnknownTransformType.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
COMPLETEDwhen 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
UNKNOWNconstant is not reachable through normal writes; on current binaries noSYSTEM.TRANSFORMrow 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 toUNKNOWN; round-trip of known values is preserved;UNKNOWNis not produced via its own serialized value.TransformDoCutoverUnknownTypeTest—doCutovershort-circuits and performs no work on the connection for anUNKNOWNrecord.TransformForceCutoverUnknownTypeTest—finishForceCutoverdoes not markCOMPLETEDand does not commit for anUNKNOWNrecord; a recognized type is still markedCOMPLETEDand committed.TransformToolValidateTransformUnknownTypeTest—validateTransformfails fast for anUNKNOWNrecord.TransformMonitorTaskUnknownTypeTest— the monitor returns aSKIPPEDresult for anUNKNOWNrecord and proceeds normally otherwise.mvn spotless:applyis 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:
SystemTransformRecordreads the transform type withgetByte()on anINTEGERcolumn. The currently used and realistic next values are small and safe; a future value>= 128could alias under the signed-byte read. The read path should move togetInt().UNKNOWNpath logs aWARNon 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))