[
https://issues.apache.org/jira/browse/IGNITE-28940?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Anton Vinogradov updated IGNITE-28940:
--------------------------------------
Description:
*Wire format change. Do this before 2.19 is released - after that it would
break rolling upgrade.*
h3. Goal
Take the marshaller from the place where we marshal, not from the message class.
h3. Why
Binary may wait for a cluster-wide class registration, and this is not allowed
on the discovery thread. So the correct marshaller depends on the transport.
The call sites are already separated:
* communication: {{GridIoManager}}, {{GridCacheIoManager}},
{{IgniteTxManager}}, {{GridDhtPartitionsExchangeFuture}},
{{CacheContinuousQueryHandler}}, {{DataStreamProcessor}}, calcite
{{MessageServiceImpl}};
* discovery: {{TcpDiscoveryIoSession}} and ZooKeeper {{DiscoveryMessageParser}}.
Hand-written code already works this way:
{{MarshallableMessage#marshal(Marshaller)}} takes the marshaller as a
parameter. Only generated code keeps it in a field.
h3. How
* add a {{Marshaller}} parameter to {{MessageMarshaller}} and
{{MessageMarshalling}}; the generator stops storing it and passes it to nested
messages;
* communication call sites pass {{ctx.marshaller()}}, discovery call sites pass
{{marshallerContext().jdkMarshaller()}};
* delete {{@UseBinaryMarshaller}},
{{AbstractMarshallableMessageFactoryProvider#init}} and both marshaller fields.
Four classes change their wire format on purpose:
* {{ErrorMessage}}, {{PartitionHashRecord}} and {{TransactionsHashRecord}}: jdk
to binary. This is safe for {{ErrorMessage}}, because {{java.lang.Throwable}}
declares {{writeObject}} and {{readObject}}. Every exception matches
{{isCustomJavaSerialization}}, so binary passes it to {{OptimizedMarshaller}}.
The bytes change, the behaviour does not.
* {{BinaryMetadataVersionInfo}}: binary to jdk, the same as
{{MetadataUpdateProposedMessage}}. Not needed if IGNITE-28939 is done first.
Note: {{MessageMarshaller}} is in the public package
{{org.apache.ignite.plugin.extensions.communication}}. Outside the generator
only three tests implement it.
h3. Expected result
No annotations left. One marshaller is used for the whole message tree of one
send, so a wrong marshaller from inheritance or from nesting is not possible.
h3. How to verify
Run All. Also continuous queries, idle_verify, {{DistributedProcess}} and
calcite. Add a test that sends the same message type over both transports.
was:
h3. Goal
Take the choice away from the message class and derive it from the place where
marshalling actually happens.
h3. Why
Whether binary may be used is decided by the call site, not by the message
type: binary blocks on a cluster-wide class registration, which is impossible
on the discovery thread. The call sites are already split by transport:
* communication - {{GridIoManager}} (marshal on send and the unmarshal paths),
{{GridCacheIoManager}}, {{IgniteTxManager}},
{{GridDhtPartitionsExchangeFuture}}, {{CacheContinuousQueryHandler}},
{{DataStreamProcessor}}, calcite {{MessageServiceImpl}};
* discovery - {{TcpDiscoveryIoSession}} and ZooKeeper
{{DiscoveryMessageParser}}.
The hand-written half of the API is already transport agnostic:
{{MarshallableMessage#marshal(Marshaller)}} takes the marshaller as a
parameter. Only the generated half keeps it in a field, which is exactly what
pins it to the class.
h3. How
* add a {{Marshaller}} parameter to {{MessageMarshaller}} and
{{MessageMarshalling}}; the generator stops storing it and passes it down into
nested messages;
* communication call sites pass {{ctx.marshaller()}}, discovery call sites pass
{{marshallerContext().jdkMarshaller()}};
* delete {{@UseBinaryMarshaller}},
{{AbstractMarshallableMessageFactoryProvider#init}} and both marshaller fields.
The wire format changes for four classes, deliberately:
* {{ErrorMessage}}, {{PartitionHashRecord}}, {{TransactionsHashRecord}} - jdk
to binary. Safe for {{ErrorMessage}}: {{java.lang.Throwable}} declares
{{writeObject}}/{{readObject}}, so every exception matches
{{isCustomJavaSerialization}} and binary delegates to {{OptimizedMarshaller}} -
the format changes, the semantics do not.
* {{BinaryMetadataVersionInfo}} - binary to jdk, aligning it with
{{MetadataUpdateProposedMessage}}; moot if the blob-removal subtask lands first.
Note: {{MessageMarshaller}} lives in the public
{{org.apache.ignite.plugin.extensions.communication}} package; outside the
generator it is implemented only by three tests.
h3. Expected result
Zero places to annotate: the marshaller is a property of the send, not of the
type. The inherited-field trap and the nested-message override disappear by
construction, because one marshaller serves the whole message tree of a single
send.
h3. How to verify
Run All; targeted runs for continuous queries, idle_verify,
{{DistributedProcess}} and calcite; plus a test that a message type marshalled
through either transport round-trips correctly.
Labels: compatibility (was: )
Summary: Choose the marshaller by transport, not by message class
(was: Select the marshaller by transport instead of per message class)
> Choose the marshaller by transport, not by message class
> --------------------------------------------------------
>
> Key: IGNITE-28940
> URL: https://issues.apache.org/jira/browse/IGNITE-28940
> Project: Ignite
> Issue Type: Sub-task
> Components: messaging
> Reporter: Anton Vinogradov
> Assignee: Anton Vinogradov
> Priority: Major
> Labels: compatibility
> Fix For: 2.19
>
>
> *Wire format change. Do this before 2.19 is released - after that it would
> break rolling upgrade.*
> h3. Goal
> Take the marshaller from the place where we marshal, not from the message
> class.
> h3. Why
> Binary may wait for a cluster-wide class registration, and this is not
> allowed on the discovery thread. So the correct marshaller depends on the
> transport. The call sites are already separated:
> * communication: {{GridIoManager}}, {{GridCacheIoManager}},
> {{IgniteTxManager}}, {{GridDhtPartitionsExchangeFuture}},
> {{CacheContinuousQueryHandler}}, {{DataStreamProcessor}}, calcite
> {{MessageServiceImpl}};
> * discovery: {{TcpDiscoveryIoSession}} and ZooKeeper
> {{DiscoveryMessageParser}}.
> Hand-written code already works this way:
> {{MarshallableMessage#marshal(Marshaller)}} takes the marshaller as a
> parameter. Only generated code keeps it in a field.
> h3. How
> * add a {{Marshaller}} parameter to {{MessageMarshaller}} and
> {{MessageMarshalling}}; the generator stops storing it and passes it to
> nested messages;
> * communication call sites pass {{ctx.marshaller()}}, discovery call sites
> pass {{marshallerContext().jdkMarshaller()}};
> * delete {{@UseBinaryMarshaller}},
> {{AbstractMarshallableMessageFactoryProvider#init}} and both marshaller
> fields.
> Four classes change their wire format on purpose:
> * {{ErrorMessage}}, {{PartitionHashRecord}} and {{TransactionsHashRecord}}:
> jdk to binary. This is safe for {{ErrorMessage}}, because
> {{java.lang.Throwable}} declares {{writeObject}} and {{readObject}}. Every
> exception matches {{isCustomJavaSerialization}}, so binary passes it to
> {{OptimizedMarshaller}}. The bytes change, the behaviour does not.
> * {{BinaryMetadataVersionInfo}}: binary to jdk, the same as
> {{MetadataUpdateProposedMessage}}. Not needed if IGNITE-28939 is done first.
> Note: {{MessageMarshaller}} is in the public package
> {{org.apache.ignite.plugin.extensions.communication}}. Outside the generator
> only three tests implement it.
> h3. Expected result
> No annotations left. One marshaller is used for the whole message tree of one
> send, so a wrong marshaller from inheritance or from nesting is not possible.
> h3. How to verify
> Run All. Also continuous queries, idle_verify, {{DistributedProcess}} and
> calcite. Add a test that sends the same message type over both transports.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
