IGNITE-27827 Use MessageSerializer for GridDeploymentInfoBean#12735
IGNITE-27827 Use MessageSerializer for GridDeploymentInfoBean#12735abashev wants to merge 6 commits intoapache:masterfrom
Conversation
...ore/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBean.java
Outdated
Show resolved
Hide resolved
| this.depMode = depMode; | ||
| this.userVer = userVer; | ||
| this.participants = participants; | ||
| this.participants = Map.copyOf(participants); |
There was a problem hiding this comment.
Why should we keep extra map in java heap?
There was a problem hiding this comment.
It is best practice for all transport protocols if you send some object from one JVM to another then need to keep it immutable as much as possible. And this one is a protection from modifying internal map and also break down a connection with original map. Up to you, if you are fine without it then I'll remove it.
| depMode = dep.deployMode(); | ||
| userVer = dep.userVersion(); | ||
| participants = dep.participants(); | ||
| participants = Map.copyOf(dep.participants()); |
There was a problem hiding this comment.
Why should we keep extra map in java heap?
| /** | ||
| * @param clsLdrId Class loader ID. | ||
| */ | ||
| public void classLoaderId(IgniteUuid clsLdrId) { |
There was a problem hiding this comment.
I suggest to keep each particular getter and setter near each other:
field1 {}
field1(field1) {}
field2() {}
field2(field2) {}
| /** */ | ||
| private static final long serialVersionUID = 0L; | ||
|
|
||
| public class GridDeploymentInfoBean implements Message, GridDeploymentInfo { |
There was a problem hiding this comment.
Have you checked that we can safely remove Externalizable? This class can be used in other Serializable classes, and removal will lead to errors in such cases.
There was a problem hiding this comment.
I did rollback for changes. I thought for RU we are removing all unversioned serializations 🤷♂️
| @@ -0,0 +1,121 @@ | |||
| package org.apache.ignite.internal.managers.deployment; | |||
There was a problem hiding this comment.
Missing license header here.
| /** | ||
| * Tests for {@link GridDeploymentInfoBean} serialization. | ||
| */ | ||
| public class GridDeploymentInfoBeanTest { |
There was a problem hiding this comment.
Such "per-message" approach seems to be very expensive. Take into account, that we have more than 300 messages, many of them have more than 10 fields.
Moreover, there are IgniteIoCommunicationMessageSerializationTest. It doesn't test different values, but it tests serialization consitency.
Also, there are integration test for continuous queries, etc.
There was a problem hiding this comment.
Code is cheap right now - if you need I can add tests for all other messages. This is raw junit without external dependencies so just a few milliseconds to overall report.
| private DeploymentMode depMode; | ||
|
|
||
| /** */ | ||
| @Order(value = 4, method = "userVersion") |
There was a problem hiding this comment.
Let's keep uniformely ascending order.
|
…/deployment/GridDeploymentInfoBean.java Co-authored-by: Ilya Shishkov <shishkovilja@gmail.com>
abashev
force-pushed
the
feature/IGNITE-27827
branch
from
5cbdea1 to
9e637d4
Compare
2 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.