Skip to content

Commit 62c51b7

Browse files
alecgrieserhatyo
andauthored
Adjust serialization of VersionValue to make continuations compatible with versions older than 4.0.564.0 (#3179)
The serialization of the `VersionValue` was modified by #2943. However, there was a flaw with the serialization, namely that it added a new field. For background, there are certain planner objects that for space reasons only get serialized fully the first time they are encountered during a query (for example, `Type` definitions). When serializing the new field in the `PVersionValue` message, if there were one of those objects, we could end up inserting a value that if referenced would later would only include the reference and not the data itself. That meant that when _deserializing_ one of these continuations on older versions, the older code would skip over the new field (not knowing what it is for) and then encounter the reference for the very first time somewhere else. Because it hadn't seen the full definition before, it didn't know what to do and threw an error. This addresses the issue by making the older serialization more closely mirror the old code. In the future, once we no longer care about being compatible with pre-4.0.564.0, we can drop the legacy mode, though we'll have to keep the deserialization path for an additional cycle. I did consider a slightly more invovled solution, where we adjust the serialization path to avoid adding to the reference registry for this field, and I got something that worked, but it ended up being a little more fiddly than I'd like, so I went with this conceptually simpler change. This does have the downside that the `Type` information gets erased after a continuation, a disadvantage the other approach would not have, but we have to be able to handle that anyway to correctly handle upgrading continuations from older versions. I've been able to validate locally that the updated continuations work, and that the failing mixed-mode tests now pass. This fixes #3178. --------- Co-authored-by: Youssef Hatem <[email protected]>
1 parent f63086b commit 62c51b7

File tree

1 file changed

+17
-4
lines changed
  • fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values

1 file changed

+17
-4
lines changed

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/VersionValue.java

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,23 @@ public boolean equals(final Object other) {
149149
@Nonnull
150150
@Override
151151
public PVersionValue toProto(@Nonnull final PlanSerializationContext serializationContext) {
152-
return PVersionValue.newBuilder()
153-
.setBaseAlias(getChildQuantifiedRecordValue().getAlias().getId()) // deprecated
154-
.setChild(childValue.toValueProto(serializationContext))
155-
.build();
152+
if (childValue instanceof QuantifiedRecordValue) {
153+
// Deprecated. This value previously required that the child be a QuantifiedRecordValue and only
154+
// contained the alias. To preserve cross-version compatibility with versions older than 4.0.561.0,
155+
// send messages without the full value.
156+
// Note: it's tempting to also include the full value in the `child` field, but doing so can
157+
// lead to mixed-mode errors. If there are types or plans referenced in the child, those can
158+
// be registered with the serializationContext, which means they may only be included
159+
// by reference elsewhere in the serialized plan. Older versions of the deserialization
160+
// logic don't know about the field, and so they will trip over any dangling references
161+
return PVersionValue.newBuilder()
162+
.setBaseAlias(getChildQuantifiedRecordValue().getAlias().getId())
163+
.build();
164+
} else {
165+
return PVersionValue.newBuilder()
166+
.setChild(childValue.toValueProto(serializationContext))
167+
.build();
168+
}
156169
}
157170

158171
@Nonnull

0 commit comments

Comments
 (0)