[jira] [Updated] (IGNITE-28935) Simplify marshaller selection for message object fields

["Anton Vinogradov (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/IGNITE-28935?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Anton Vinogradov updated IGNITE-28935:
--------------------------------------
    Description: 
h3. Goal

Make it obvious - and mechanically checkable - which marshaller every message field is serialized with, and cut the number of places where that decision is taken from 60 to zero.

h3. Why

Object fields of messages are turned into bytes either by the schema-less {{JdkMarshaller}} or by the schema-aware {{BinaryMarshaller}}. The marshaller is bound to the message *class*: the {{@UseBinaryMarshaller}} annotation plus the registration call in {{CoreMessagesProvider}}.

Measured on master over the generated companions:
* 297 messages are registered in core (58 binary, 239 jdk), 195 {{*Marshaller}} companions are generated;
* only 30 companions hold a {{Marshaller}} at all, and the choice affects 24 classes;
* of 60 {{@UseBinaryMarshaller}} annotations only 13 mean anything - the other 47 are noise.

The annotation is meant to say "the message carries user classes", but that is not what it does. User data travels with the jdk marshaller in {{TcpDiscoveryNode#attrs}}, {{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}}, {{QueryEntityMessage#dfltFieldValues}}, {{PartitionHashRecord#consistentId}}. What actually decides is whether the marshalling may block on a cluster-wide class registration: binary - and {{OptimizedMarshaller}} inside it - goes {{MarshallerContextImpl#registerClassName}} -> {{proposeMapping}} -> {{fut.get()}}, which is impossible on the discovery thread, during join, and for on-disk formats read by offline tools. That is a property of the call site, not of the class.

Binding it to the class produces three defects:

# The annotation is not {{@Inherited}}, while the generator builds a companion from all fields including inherited ones ({{MessageCompanionGenerator#enclosedFields}}). Nine parent/child pairs are registered in different categories. Verified by adding a blob field to {{GridDistributedTxPrepareRequest}} (binary): in the generated {{GridNearTxPrepareRequestMarshaller}} (jdk) the very same inherited field is marshalled with jdk. No such field exists today, so this is a latent trap rather than a live bug.
# A nested message overrides the enclosing one, because the marshaller is resolved per {{directType}}: {{ErrorMessage}} (jdk) travels inside binary messages, and {{BinaryMetadataVersionInfo}} (binary) travels inside the jdk discovery data bag {{CacheBinaryDataBagItem}} - while the same {{BinaryMetadata}} in {{MetadataUpdateProposedMessage}} is marshalled with jdk. It works only because those classes are pre-registered in {{META-INF/classnames.properties}} and need no ring round-trip.
# {{MarshallableMessage}} conflates "needs a pre-marshal hook" with "needs a marshaller": 8 of its 21 implementations use neither the marshaller nor the class loader.

h3. Plan

Each subtask is independently mergeable and useful on its own.

# IGNITE-28936 - *Remove dead marshalling code from {{GridDhtAtomicUpdateRequest}}.* {{forceTransformBackups}} is {{false}} at both creation sites, so the hand-written marshalling and the fields it fills are unreachable. Result: one boolean and three always-null collection fields leave the wire, and one of the 11 meaningful annotations goes away.
# IGNITE-28937 - *Register core messages uniformly.* {{withSchema}}/{{withNoSchema}} are identical after IGNITE-28929, and the marshaller is threaded into serializer and deployer lookups that never take one. Result: a single registration form, the marshaller stated once, generated code byte-identical.
# IGNITE-28938 - *Split {{MarshallableMessage}} into a hook and a marshalling contract.* 8 of its 21 implementations use neither the marshaller nor the class loader - they only reshape their own fields. Result: 'needs a marshaller' becomes visible in the type, and 8 companions lose the field.
# IGNITE-28939 - *Replace marshaller blobs of internal types with regular message fields* - {{BinaryMetadata}} (two messages), {{BaselineTopology}}, job siblings, query entities. Binary yields no schema for them anyway, since {{Externalizable}} types fall back to {{OptimizedMarshaller}} inside it. Result: blob fields 24 -> ~19, and the {{BinaryMetadata}} inconsistency disappears.
# IGNITE-28940 - *Select the marshaller by transport instead of per message class* - the core change. Communication call sites pass binary, discovery call sites pass jdk, and the generator passes it down the message tree. Result: zero annotations, the inherited-field trap and the nested-message override become impossible, and the wire format changes for four classes deliberately.
# IGNITE-28941 - *Test that discovery messages never require cluster-wide class registration.* Today the rule holds by luck in at least one place - only because the classes involved are pre-registered in {{classnames.properties}}. Result: the rule is checked in CI instead of remembered.

Order: the first two are cheap and independent of everything else; the hook split and the blob removal both shrink the set of classes the transport change has to touch, so they come before it; the test closes the loop last.

h3. Expected result

|| || before || after ||
| places where the marshaller is chosen | 60 annotations, two registration helpers | 0 annotations, marshaller comes from the call site |
| companions holding a {{Marshaller}} | 30 | ~12, hand-written marshalling only |
| classes given a marshaller they never use | 6 | 0 |
| blob fields of internal types | 24 | ~19 |
| inherited-field and nested-message mismatches | possible | impossible by construction |

The number of marshaller implementations does not change - all three are load-bearing: jdk serializes without any cluster context, binary gives field access without the class, and optimized is the fallback binary itself relies on for {{Externalizable}} and {{writeObject}}/{{readObject}} types (and the REST client protocol).

  was:
h3. Goal

Make it obvious - and mechanically checkable - which marshaller every message field is serialized with, and cut the number of places where that decision is taken from 60 to zero.

h3. Why

Object fields of messages are turned into bytes either by the schema-less {{JdkMarshaller}} or by the schema-aware {{BinaryMarshaller}}. The marshaller is bound to the message *class*: the {{@UseBinaryMarshaller}} annotation plus the registration call in {{CoreMessagesProvider}}.

Measured on master over the generated companions:
* 297 messages are registered in core (58 binary, 239 jdk), 195 {{*Marshaller}} companions are generated;
* only 30 companions hold a {{Marshaller}} at all, and the choice affects 24 classes;
* of 60 {{@UseBinaryMarshaller}} annotations only 13 mean anything - the other 47 are noise.

The annotation is meant to say "the message carries user classes", but that is not what it does. User data travels with the jdk marshaller in {{TcpDiscoveryNode#attrs}}, {{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}}, {{QueryEntityMessage#dfltFieldValues}}, {{PartitionHashRecord#consistentId}}. What actually decides is whether the marshalling may block on a cluster-wide class registration: binary - and {{OptimizedMarshaller}} inside it - goes {{MarshallerContextImpl#registerClassName}} -> {{proposeMapping}} -> {{fut.get()}}, which is impossible on the discovery thread, during join, and for on-disk formats read by offline tools. That is a property of the call site, not of the class.

Binding it to the class produces three defects:

# The annotation is not {{@Inherited}}, while the generator builds a companion from all fields including inherited ones ({{MessageCompanionGenerator#enclosedFields}}). Nine parent/child pairs are registered in different categories. Verified by adding a blob field to {{GridDistributedTxPrepareRequest}} (binary): in the generated {{GridNearTxPrepareRequestMarshaller}} (jdk) the very same inherited field is marshalled with jdk. No such field exists today, so this is a latent trap rather than a live bug.
# A nested message overrides the enclosing one, because the marshaller is resolved per {{directType}}: {{ErrorMessage}} (jdk) travels inside binary messages, and {{BinaryMetadataVersionInfo}} (binary) travels inside the jdk discovery data bag {{CacheBinaryDataBagItem}} - while the same {{BinaryMetadata}} in {{MetadataUpdateProposedMessage}} is marshalled with jdk. It works only because those classes are pre-registered in {{META-INF/classnames.properties}} and need no ring round-trip.
# {{MarshallableMessage}} conflates "needs a pre-marshal hook" with "needs a marshaller": 8 of its 21 implementations use neither the marshaller nor the class loader.

h3. Plan

Each subtask is independently mergeable and useful on its own.

# (pending) - *Remove dead marshalling code from {{GridDhtAtomicUpdateRequest}}.* {{forceTransformBackups}} is {{false}} at both creation sites, so the hand-written marshalling and the fields it fills are unreachable. Result: one boolean and three always-null collection fields leave the wire, and one of the 11 meaningful annotations goes away.
# (pending) - *Register core messages uniformly.* {{withSchema}}/{{withNoSchema}} are identical after IGNITE-28929, and the marshaller is threaded into serializer and deployer lookups that never take one. Result: a single registration form, the marshaller stated once, generated code byte-identical.
# (pending) - *Split {{MarshallableMessage}} into a hook and a marshalling contract.* 8 of its 21 implementations use neither the marshaller nor the class loader - they only reshape their own fields. Result: 'needs a marshaller' becomes visible in the type, and 8 companions lose the field.
# (pending) - *Replace marshaller blobs of internal types with regular message fields* - {{BinaryMetadata}} (two messages), {{BaselineTopology}}, job siblings, query entities. Binary yields no schema for them anyway, since {{Externalizable}} types fall back to {{OptimizedMarshaller}} inside it. Result: blob fields 24 -> ~19, and the {{BinaryMetadata}} inconsistency disappears.
# (pending) - *Select the marshaller by transport instead of per message class* - the core change. Communication call sites pass binary, discovery call sites pass jdk, and the generator passes it down the message tree. Result: zero annotations, the inherited-field trap and the nested-message override become impossible, and the wire format changes for four classes deliberately.
# (pending) - *Test that discovery messages never require cluster-wide class registration.* Today the rule holds by luck in at least one place - only because the classes involved are pre-registered in {{classnames.properties}}. Result: the rule is checked in CI instead of remembered.

Order: the first two are cheap and independent of everything else; the hook split and the blob removal both shrink the set of classes the transport change has to touch, so they come before it; the test closes the loop last.

h3. Expected result

|| || before || after ||
| places where the marshaller is chosen | 60 annotations, two registration helpers | 0 annotations, marshaller comes from the call site |
| companions holding a {{Marshaller}} | 30 | ~12, hand-written marshalling only |
| classes given a marshaller they never use | 6 | 0 |
| blob fields of internal types | 24 | ~19 |
| inherited-field and nested-message mismatches | possible | impossible by construction |

The number of marshaller implementations does not change - all three are load-bearing: jdk serializes without any cluster context, binary gives field access without the class, and optimized is the fallback binary itself relies on for {{Externalizable}} and {{writeObject}}/{{readObject}} types (and the REST client protocol).


> Simplify marshaller selection for message object fields
> -------------------------------------------------------
>
>                 Key: IGNITE-28935
>                 URL: https://issues.apache.org/jira/browse/IGNITE-28935
>             Project: Ignite
>          Issue Type: Task
>          Components: messaging
>            Reporter: Anton Vinogradov
>            Assignee: Anton Vinogradov
>            Priority: Major
>             Fix For: 2.19
>
>
> h3. Goal
> Make it obvious - and mechanically checkable - which marshaller every message field is serialized with, and cut the number of places where that decision is taken from 60 to zero.
> h3. Why
> Object fields of messages are turned into bytes either by the schema-less {{JdkMarshaller}} or by the schema-aware {{BinaryMarshaller}}. The marshaller is bound to the message *class*: the {{@UseBinaryMarshaller}} annotation plus the registration call in {{CoreMessagesProvider}}.
> Measured on master over the generated companions:
> * 297 messages are registered in core (58 binary, 239 jdk), 195 {{*Marshaller}} companions are generated;
> * only 30 companions hold a {{Marshaller}} at all, and the choice affects 24 classes;
> * of 60 {{@UseBinaryMarshaller}} annotations only 13 mean anything - the other 47 are noise.
> The annotation is meant to say "the message carries user classes", but that is not what it does. User data travels with the jdk marshaller in {{TcpDiscoveryNode#attrs}}, {{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}}, {{QueryEntityMessage#dfltFieldValues}}, {{PartitionHashRecord#consistentId}}. What actually decides is whether the marshalling may block on a cluster-wide class registration: binary - and {{OptimizedMarshaller}} inside it - goes {{MarshallerContextImpl#registerClassName}} -> {{proposeMapping}} -> {{fut.get()}}, which is impossible on the discovery thread, during join, and for on-disk formats read by offline tools. That is a property of the call site, not of the class.
> Binding it to the class produces three defects:
> # The annotation is not {{@Inherited}}, while the generator builds a companion from all fields including inherited ones ({{MessageCompanionGenerator#enclosedFields}}). Nine parent/child pairs are registered in different categories. Verified by adding a blob field to {{GridDistributedTxPrepareRequest}} (binary): in the generated {{GridNearTxPrepareRequestMarshaller}} (jdk) the very same inherited field is marshalled with jdk. No such field exists today, so this is a latent trap rather than a live bug.
> # A nested message overrides the enclosing one, because the marshaller is resolved per {{directType}}: {{ErrorMessage}} (jdk) travels inside binary messages, and {{BinaryMetadataVersionInfo}} (binary) travels inside the jdk discovery data bag {{CacheBinaryDataBagItem}} - while the same {{BinaryMetadata}} in {{MetadataUpdateProposedMessage}} is marshalled with jdk. It works only because those classes are pre-registered in {{META-INF/classnames.properties}} and need no ring round-trip.
> # {{MarshallableMessage}} conflates "needs a pre-marshal hook" with "needs a marshaller": 8 of its 21 implementations use neither the marshaller nor the class loader.
> h3. Plan
> Each subtask is independently mergeable and useful on its own.
> # IGNITE-28936 - *Remove dead marshalling code from {{GridDhtAtomicUpdateRequest}}.* {{forceTransformBackups}} is {{false}} at both creation sites, so the hand-written marshalling and the fields it fills are unreachable. Result: one boolean and three always-null collection fields leave the wire, and one of the 11 meaningful annotations goes away.
> # IGNITE-28937 - *Register core messages uniformly.* {{withSchema}}/{{withNoSchema}} are identical after IGNITE-28929, and the marshaller is threaded into serializer and deployer lookups that never take one. Result: a single registration form, the marshaller stated once, generated code byte-identical.
> # IGNITE-28938 - *Split {{MarshallableMessage}} into a hook and a marshalling contract.* 8 of its 21 implementations use neither the marshaller nor the class loader - they only reshape their own fields. Result: 'needs a marshaller' becomes visible in the type, and 8 companions lose the field.
> # IGNITE-28939 - *Replace marshaller blobs of internal types with regular message fields* - {{BinaryMetadata}} (two messages), {{BaselineTopology}}, job siblings, query entities. Binary yields no schema for them anyway, since {{Externalizable}} types fall back to {{OptimizedMarshaller}} inside it. Result: blob fields 24 -> ~19, and the {{BinaryMetadata}} inconsistency disappears.
> # IGNITE-28940 - *Select the marshaller by transport instead of per message class* - the core change. Communication call sites pass binary, discovery call sites pass jdk, and the generator passes it down the message tree. Result: zero annotations, the inherited-field trap and the nested-message override become impossible, and the wire format changes for four classes deliberately.
> # IGNITE-28941 - *Test that discovery messages never require cluster-wide class registration.* Today the rule holds by luck in at least one place - only because the classes involved are pre-registered in {{classnames.properties}}. Result: the rule is checked in CI instead of remembered.
> Order: the first two are cheap and independent of everything else; the hook split and the blob removal both shrink the set of classes the transport change has to touch, so they come before it; the test closes the loop last.
> h3. Expected result
> || || before || after ||
> | places where the marshaller is chosen | 60 annotations, two registration helpers | 0 annotations, marshaller comes from the call site |
> | companions holding a {{Marshaller}} | 30 | ~12, hand-written marshalling only |
> | classes given a marshaller they never use | 6 | 0 |
> | blob fields of internal types | 24 | ~19 |
> | inherited-field and nested-message mismatches | possible | impossible by construction |
> The number of marshaller implementations does not change - all three are load-bearing: jdk serializes without any cluster context, binary gives field access without the class, and optimized is the fallback binary itself relies on for {{Externalizable}} and {{writeObject}}/{{readObject}} types (and the REST client protocol).



--
This message was sent by Atlassian Jira
(v8.20.10#820010)