[
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 cannot be used where the marshalling cannot afford a cluster-wide class
registration, and that is a property of the call site.
The registration itself is {{MarshallerContextImpl#registerClassName}} ->
{{proposeMapping}} -> {{fut.get()}}, which waits for discovery. Ignite classes
usually skip it, because 2276 names are pre-accepted from
{{META-INF/classnames.properties}}. But that list is not closed under nesting:
a plain {{CacheConfiguration}} needs 9 types registered and two of them are
absent - {{javax.cache.configuration.FactoryBuilder$SingletonFactory}} and
{{javax.cache.expiry.EternalExpiryPolicy}}, both coming from the default expiry
policy. A discovery message carrying a cache configuration would therefore
register types from inside the discovery thread.
Measured on the same objects, binary is the better marshaller *when types are
registered*: node attributes 2423 bytes / 5.2 us against 2725 / 30.0 for jdk,
{{CacheConfiguration}} 857 bytes against 4539. So the split is not "binary is
for user data" - it is "binary needs a place where registration is allowed".
The call sites are already separated by transport, and transport is a reliable
indicator of that:
* 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 already works this way:
{{MarshallableMessage#marshal(Marshaller)}} takes the marshaller as a
parameter. Only generated code keeps it in a field, which is what pins it to
the class.
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.
The wire format changes for four classes, deliberately:
* {{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}}:
709 bytes with jdk against 715 with binary, 6.7 us against 7.4. 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.
When the set of pre-registered types changes, or a self-contained binary mode
appears, only the call sites have to be revisited - not sixty classes.
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:
*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.
> 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 cannot be used where the marshalling cannot afford a cluster-wide
> class registration, and that is a property of the call site.
> The registration itself is {{MarshallerContextImpl#registerClassName}} ->
> {{proposeMapping}} -> {{fut.get()}}, which waits for discovery. Ignite
> classes usually skip it, because 2276 names are pre-accepted from
> {{META-INF/classnames.properties}}. But that list is not closed under
> nesting: a plain {{CacheConfiguration}} needs 9 types registered and two of
> them are absent -
> {{javax.cache.configuration.FactoryBuilder$SingletonFactory}} and
> {{javax.cache.expiry.EternalExpiryPolicy}}, both coming from the default
> expiry policy. A discovery message carrying a cache configuration would
> therefore register types from inside the discovery thread.
> Measured on the same objects, binary is the better marshaller *when types are
> registered*: node attributes 2423 bytes / 5.2 us against 2725 / 30.0 for jdk,
> {{CacheConfiguration}} 857 bytes against 4539. So the split is not "binary is
> for user data" - it is "binary needs a place where registration is allowed".
> The call sites are already separated by transport, and transport is a
> reliable indicator of that:
> * 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 already works this way:
> {{MarshallableMessage#marshal(Marshaller)}} takes the marshaller as a
> parameter. Only generated code keeps it in a field, which is what pins it to
> the class.
> 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.
> The wire format changes for four classes, deliberately:
> * {{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}}: 709 bytes with jdk against 715 with binary, 6.7 us
> against 7.4. 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.
> When the set of pre-registered types changes, or a self-contained binary mode
> appears, only the call sites have to be revisited - not sixty classes.
> 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)
