[
https://issues.apache.org/jira/browse/IGNITE-26631?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138566#comment-18138566
]
Reza Taroosheh commented on IGNITE-26631:
-----------------------------------------
Hi [~vladsz83], here are the answers to your questions. I tested them with
prototypes in a separate copy of master (7b4a2a91fd), so nothing here is in a
PR.
*1. Can we remove one of the messages?*
Yes, {{GridCacheGroupIdMessage}} can be removed without changing the bytes sent
between nodes. No message has both IDs, so the 4 group messages can extend
{{GridCacheIdMessage}} and keep the group ID in {{cacheId}}.
* The bytes were identical for the 8 messages I compared (4 group, 4 cache).
* A two-node cluster with one master node and one prototype node passed in both
start orders, with rebalancing and updates.
* Only 4 wire schema files change, and only the field name.
The gain is small, though. The IDs are the same only for a cache without a
group. If caches {{A}} and {{B}} are in group {{G}}, there are three different
IDs, and a rebalancing message carries {{G}}'s ID because it covers both
caches. So the code still has to know which kind of ID it received. In the
prototype, the group messages still needed a marker interface to identify them,
so {{GridCacheIoManager}} has the same number of {{instanceof}} checks on these
types (6).
*2. Can a boolean flag replace {{instanceof}}?*
It can, but I don't suggest it. Every cache message grows by 1 byte (8 of 8
measured), and 45 message schemas change. Without a feature gate, a master node
couldn't read the prototype's messages ({{UnknownMessageException}}), so it
would need a new {{IgniteFeature}} and compatibility tests.
*3. Does every {{GridCacheMessage}} need {{descriptor()}}?*
No. {{descriptorForMessage()}} is only used in 2 log messages, and messages
like {{TxLocksRequest}} have no ID. It doesn't answer the main question, so I
suggest closing #13652.
*My suggestion:* keep both classes, as removing one adds little. If you want to
remove the duplicate ID field, a common parent class could hold the one
{{int}}, and both classes would stay. It also shortens the ID lookup in
{{handleMessage()}} to one {{instanceof}} check. It needs less logic change
than removing a class, because the handler bounds and the generator stay as
they are, but it renames the field in about 45 schemas. I haven't tested that
one, but the {{int}} would stay in the same position.
*Not tested:* the full test suite, {{modules/compatibility}}, different
releases, persistence, client nodes, and stopping a cache during rebalancing.
*Recap:*
# Removing the duplicate IDs, with a marker interface or a common parent,
doesn't remove all the {{instanceof}} checks, because some places still need to
know whether a message has a cache ID or a group ID.
# A boolean flag changes the wire format, so it needs a new {{IgniteFeature}}
and compatibility tests.
# Not every {{GridCacheMessage}} needs {{descriptor()}}, so I suggest closing
#13652.
Which do you prefer: keep both, remove one, or add a common parent?
> Combine or refactor GridCacheGroupIdMessage and GridCacheIdMessage
> ------------------------------------------------------------------
>
> Key: IGNITE-26631
> URL: https://issues.apache.org/jira/browse/IGNITE-26631
> Project: Ignite
> Issue Type: Improvement
> Reporter: Vladimir Steshin
> Assignee: Reza Taroosheh
> Priority: Minor
> Labels: IEP-132, ise, wire-format
> Fix For: 2.19
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> These messages are similar and are only for providing a cache or a group id
> to find proper cache message handler. Currently, we use `{_}instance of{_}`.
> Instead, we should revise these messages and probably combine them.
>
> Also, check in {_}GridCacheIoManager{_}:
> {code:java}
> private Object descriptorForMessage(GridCacheMessage msg) {
> if (msg instanceof GridCacheIdMessage)
> return
> cctx.cache().cacheDescriptor(((GridCacheIdMessage)msg).cacheId());
> else if (msg instanceof GridCacheGroupIdMessage)
> return
> cctx.cache().cacheGroupDescriptors().get(((GridCacheGroupIdMessage)msg).groupId());
> return null;
> } {code}
>
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
