[
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 clear which marshaller is used for each message field. Today this is
set in 60 places. It should be set in none.
h3. Why
An object field of a message becomes bytes either by {{JdkMarshaller}} or by
{{BinaryMarshaller}}. The marshaller is chosen per message class: the
{{@UseBinaryMarshaller}} annotation plus the call in {{CoreMessagesProvider}}.
Numbers from master, counted in the generated companions:
* core registers 297 messages: 58 binary, 239 jdk;
* 195 {{*Marshaller}} companions are generated, but only 30 keep a
{{Marshaller}};
* the choice changes the result for 24 classes only;
* there are 60 annotations, and only 13 of them matter. The other 47 do nothing.
The annotation should mean "this message carries user classes". It does not.
User data goes with jdk in {{TcpDiscoveryNode#attrs}},
{{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}} and
{{QueryEntityMessage#dfltFieldValues}}.
The real rule is different. When binary sees a class for the first time, it
registers the class name in the whole cluster:
{{MarshallerContextImpl#registerClassName}} -> {{proposeMapping}} ->
{{fut.get()}}. This call waits for discovery. So it cannot be used on the
discovery thread, during node join, or for files that must be read without a
cluster. This depends on where we marshal, not on the message type.
Binding the marshaller to the class gives three problems:
# The annotation is not {{@Inherited}}, but the generator builds a companion
from all fields, including inherited ones. Nine parent/child pairs are
registered in different groups. Checked with an experiment: add a blob field to
{{GridDistributedTxPrepareRequest}} (binary), and the generated
{{GridNearTxPrepareRequestMarshaller}} (jdk) marshals the same field with jdk.
No such field exists today, so this is a trap, not a live bug.
# A nested message overrides the outer one, because the marshaller is taken by
{{directType}}. {{ErrorMessage}} (jdk) travels inside binary messages.
{{BinaryMetadataVersionInfo}} (binary) travels inside the jdk discovery data
bag. The same {{BinaryMetadata}} in {{MetadataUpdateProposedMessage}} uses jdk.
This works only because such classes are listed in
{{META-INF/classnames.properties}} and need no cluster call.
# {{MarshallableMessage}} means two things at once: "call me before send" and
"I need a marshaller". 8 of its 21 implementations use neither the marshaller
nor the class loader.
h3. Plan
Three subtasks change the bytes on the wire. They should be done *first*, while
2.19 is not released yet. After the release the same changes would break
rolling upgrade and would need a feature flag plus support for both formats.
*Do first - these change the wire format:*
# IGNITE-28936 - *remove dead marshalling in {{GridDhtAtomicUpdateRequest}}.*
The flag is always {{false}}, so this code never runs. Result: 4 fields less on
the wire.
# IGNITE-28939 - *turn blob fields of internal types into normal message
fields* ({{BinaryMetadata}}, {{BaselineTopology}}, job siblings, query
entities). Result: 24 blob fields become about 19, and the {{BinaryMetadata}}
mismatch is gone.
# IGNITE-28940 - *pass the marshaller from the call site.* This is the main
change. Communication passes binary, discovery passes jdk. Four classes change
their format on purpose. Result: no annotations left, and a wrong marshaller
becomes impossible.
*Can be done any time - no wire format change:*
# IGNITE-28937 - *one registration form in {{CoreMessagesProvider}}.*
{{withSchema}} and {{withNoSchema}} are the same code now. Result: the
marshaller is written in one place, generated code does not change.
# IGNITE-28938 - *split {{MarshallableMessage}}.* 8 of 21 classes only need a
callback. Result: they stop getting a marshaller. Small task: if it is done
before IGNITE-28940, that one becomes smaller.
# IGNITE-28941 - *add a test for the discovery path.* Result: the rule is
checked by CI, not kept in mind.
h3. Expected result
|| || now || after ||
| places that choose the marshaller | 60 annotations | 0 |
| companions that keep a {{Marshaller}} | 30 | about 12 |
| classes that get a marshaller and never use it | 6 | 0 |
| blob fields of internal types | 24 | about 19 |
| wrong marshaller from inheritance or nesting | possible | not possible |
All three marshaller implementations are still needed. jdk works without a
cluster. binary reads fields without the class. optimized is used by binary
itself for {{Externalizable}} classes, and by the REST 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.
# 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).
Summary: Simplify how the marshaller is chosen for message fields
(was: Simplify marshaller selection for message object fields)
> Simplify how the marshaller is chosen for message 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 clear which marshaller is used for each message field. Today this is
> set in 60 places. It should be set in none.
> h3. Why
> An object field of a message becomes bytes either by {{JdkMarshaller}} or by
> {{BinaryMarshaller}}. The marshaller is chosen per message class: the
> {{@UseBinaryMarshaller}} annotation plus the call in {{CoreMessagesProvider}}.
> Numbers from master, counted in the generated companions:
> * core registers 297 messages: 58 binary, 239 jdk;
> * 195 {{*Marshaller}} companions are generated, but only 30 keep a
> {{Marshaller}};
> * the choice changes the result for 24 classes only;
> * there are 60 annotations, and only 13 of them matter. The other 47 do
> nothing.
> The annotation should mean "this message carries user classes". It does not.
> User data goes with jdk in {{TcpDiscoveryNode#attrs}},
> {{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}} and
> {{QueryEntityMessage#dfltFieldValues}}.
> The real rule is different. When binary sees a class for the first time, it
> registers the class name in the whole cluster:
> {{MarshallerContextImpl#registerClassName}} -> {{proposeMapping}} ->
> {{fut.get()}}. This call waits for discovery. So it cannot be used on the
> discovery thread, during node join, or for files that must be read without a
> cluster. This depends on where we marshal, not on the message type.
> Binding the marshaller to the class gives three problems:
> # The annotation is not {{@Inherited}}, but the generator builds a companion
> from all fields, including inherited ones. Nine parent/child pairs are
> registered in different groups. Checked with an experiment: add a blob field
> to {{GridDistributedTxPrepareRequest}} (binary), and the generated
> {{GridNearTxPrepareRequestMarshaller}} (jdk) marshals the same field with
> jdk. No such field exists today, so this is a trap, not a live bug.
> # A nested message overrides the outer one, because the marshaller is taken
> by {{directType}}. {{ErrorMessage}} (jdk) travels inside binary messages.
> {{BinaryMetadataVersionInfo}} (binary) travels inside the jdk discovery data
> bag. The same {{BinaryMetadata}} in {{MetadataUpdateProposedMessage}} uses
> jdk. This works only because such classes are listed in
> {{META-INF/classnames.properties}} and need no cluster call.
> # {{MarshallableMessage}} means two things at once: "call me before send" and
> "I need a marshaller". 8 of its 21 implementations use neither the marshaller
> nor the class loader.
> h3. Plan
> Three subtasks change the bytes on the wire. They should be done *first*,
> while 2.19 is not released yet. After the release the same changes would
> break rolling upgrade and would need a feature flag plus support for both
> formats.
> *Do first - these change the wire format:*
> # IGNITE-28936 - *remove dead marshalling in {{GridDhtAtomicUpdateRequest}}.*
> The flag is always {{false}}, so this code never runs. Result: 4 fields less
> on the wire.
> # IGNITE-28939 - *turn blob fields of internal types into normal message
> fields* ({{BinaryMetadata}}, {{BaselineTopology}}, job siblings, query
> entities). Result: 24 blob fields become about 19, and the {{BinaryMetadata}}
> mismatch is gone.
> # IGNITE-28940 - *pass the marshaller from the call site.* This is the main
> change. Communication passes binary, discovery passes jdk. Four classes
> change their format on purpose. Result: no annotations left, and a wrong
> marshaller becomes impossible.
> *Can be done any time - no wire format change:*
> # IGNITE-28937 - *one registration form in {{CoreMessagesProvider}}.*
> {{withSchema}} and {{withNoSchema}} are the same code now. Result: the
> marshaller is written in one place, generated code does not change.
> # IGNITE-28938 - *split {{MarshallableMessage}}.* 8 of 21 classes only need a
> callback. Result: they stop getting a marshaller. Small task: if it is done
> before IGNITE-28940, that one becomes smaller.
> # IGNITE-28941 - *add a test for the discovery path.* Result: the rule is
> checked by CI, not kept in mind.
> h3. Expected result
> || || now || after ||
> | places that choose the marshaller | 60 annotations | 0 |
> | companions that keep a {{Marshaller}} | 30 | about 12 |
> | classes that get a marshaller and never use it | 6 | 0 |
> | blob fields of internal types | 24 | about 19 |
> | wrong marshaller from inheritance or nesting | possible | not possible |
> All three marshaller implementations are still needed. jdk works without a
> cluster. binary reads fields without the class. optimized is used by binary
> itself for {{Externalizable}} classes, and by the REST protocol.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
