shishkovilja commented on code in PR #12735:
URL: https://github.com/apache/ignite/pull/12735#discussion_r2803742850
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBean.java:
##########
@@ -18,46 +18,39 @@
package org.apache.ignite.internal.managers.deployment;
import java.io.Externalizable;
-import java.io.IOException;
-import java.io.ObjectInput;
-import java.io.ObjectOutput;
-import java.nio.ByteBuffer;
import java.util.Map;
import java.util.UUID;
import org.apache.ignite.configuration.DeploymentMode;
-import org.apache.ignite.internal.GridDirectMap;
+import org.apache.ignite.internal.Order;
import org.apache.ignite.internal.util.tostring.GridToStringInclude;
import org.apache.ignite.internal.util.typedef.internal.S;
-import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.lang.IgniteUuid;
import org.apache.ignite.plugin.extensions.communication.Message;
-import
org.apache.ignite.plugin.extensions.communication.MessageCollectionItemType;
-import org.apache.ignite.plugin.extensions.communication.MessageReader;
-import org.apache.ignite.plugin.extensions.communication.MessageWriter;
/**
* Deployment info bean.
*/
-public class GridDeploymentInfoBean implements Message, GridDeploymentInfo,
Externalizable {
- /** */
- private static final long serialVersionUID = 0L;
-
+public class GridDeploymentInfoBean implements Message, GridDeploymentInfo {
/** */
+ @Order(value = 0, method = "classLoaderId")
private IgniteUuid clsLdrId;
/** */
+ @Order(value = 1, method = "deployMode")
private DeploymentMode depMode;
/** */
+ @Order(value = 4, method = "userVersion")
private String userVer;
/** */
+ @Order(value = 2, method = "localDeploymentOwner")
@Deprecated // Left for backward compatibility only.
private boolean locDepOwner;
/** Node class loader participant map. */
@GridToStringInclude
- @GridDirectMap(keyType = UUID.class, valueType = IgniteUuid.class)
+ @Order(value = 3, method = "participants")
Review Comment:
When method and field contains same name, we can skip explicit method
specifying.
```suggestion
@Order(3)
```
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBean.java:
##########
@@ -125,139 +118,55 @@ public GridDeploymentInfoBean(GridDeploymentInfo dep) {
return participants;
}
- /** {@inheritDoc} */
- @Override public int hashCode() {
- return clsLdrId.hashCode();
+ /**
+ * @param clsLdrId Class loader ID.
+ */
+ public void classLoaderId(IgniteUuid clsLdrId) {
Review Comment:
I suggest to keep each particular getter and setter near each other:
field1 {}
field1(field1) {}
field2() {}
field2(field2) {}
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBean.java:
##########
@@ -18,46 +18,39 @@
package org.apache.ignite.internal.managers.deployment;
import java.io.Externalizable;
-import java.io.IOException;
-import java.io.ObjectInput;
-import java.io.ObjectOutput;
-import java.nio.ByteBuffer;
import java.util.Map;
import java.util.UUID;
import org.apache.ignite.configuration.DeploymentMode;
-import org.apache.ignite.internal.GridDirectMap;
+import org.apache.ignite.internal.Order;
import org.apache.ignite.internal.util.tostring.GridToStringInclude;
import org.apache.ignite.internal.util.typedef.internal.S;
-import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.lang.IgniteUuid;
import org.apache.ignite.plugin.extensions.communication.Message;
-import
org.apache.ignite.plugin.extensions.communication.MessageCollectionItemType;
-import org.apache.ignite.plugin.extensions.communication.MessageReader;
-import org.apache.ignite.plugin.extensions.communication.MessageWriter;
/**
* Deployment info bean.
*/
-public class GridDeploymentInfoBean implements Message, GridDeploymentInfo,
Externalizable {
- /** */
- private static final long serialVersionUID = 0L;
-
+public class GridDeploymentInfoBean implements Message, GridDeploymentInfo {
Review Comment:
Have you checked that we can safely remove `Externalizable`? This class can
be used in other `Serializable` classed, and removal will lead to errors.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBean.java:
##########
@@ -82,7 +75,7 @@ public GridDeploymentInfoBean(
this.clsLdrId = clsLdrId;
this.depMode = depMode;
this.userVer = userVer;
- this.participants = participants;
+ this.participants = Map.copyOf(participants);
Review Comment:
Why should we keep extra map in java heap?
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBeanTest.java:
##########
@@ -0,0 +1,121 @@
+package org.apache.ignite.internal.managers.deployment;
Review Comment:
Misssing license header here.
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBeanTest.java:
##########
@@ -0,0 +1,121 @@
+package org.apache.ignite.internal.managers.deployment;
+
+import static java.util.UUID.randomUUID;
+import static org.apache.ignite.lang.IgniteUuid.randomUuid;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.nio.ByteBuffer;
+import java.util.Map;
+import java.util.function.Function;
+import org.apache.ignite.configuration.DeploymentMode;
+import org.apache.ignite.internal.direct.DirectMessageReader;
+import org.apache.ignite.internal.direct.DirectMessageWriter;
+import org.apache.ignite.internal.managers.communication.GridIoMessageFactory;
+import
org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl;
+import org.apache.ignite.plugin.extensions.communication.Message;
+import org.apache.ignite.plugin.extensions.communication.MessageFactory;
+import
org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider;
+import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import org.junit.Test;
+
+/**
+ * Tests for {@link GridDeploymentInfoBean} serialization.
+ */
+public class GridDeploymentInfoBeanTest {
Review 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.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentInfoBean.java:
##########
@@ -92,7 +85,7 @@ public GridDeploymentInfoBean(GridDeploymentInfo dep) {
clsLdrId = dep.classLoaderId();
depMode = dep.deployMode();
userVer = dep.userVersion();
- participants = dep.participants();
+ participants = Map.copyOf(dep.participants());
Review Comment:
Why should we keep extra map in java heap?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
