Vladsz83 commented on code in PR #12878:
URL: https://github.com/apache/ignite/pull/12878#discussion_r2931876967
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/DiscoveryMessageFactory.java:
##########
@@ -132,23 +132,21 @@
import
org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryServerOnlyCustomEventMessageSerializer;
import
org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryStatusCheckMessage;
import
org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryStatusCheckMessageSerializer;
-import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.NotNull;
/** Message factory for discovery messages. */
public class DiscoveryMessageFactory implements MessageFactoryProvider {
/** Custom data marshaller. */
- private final @Nullable Marshaller cstDataMarshall;
+ private final @NotNull Marshaller cstDataMarshall;
/** Class loader for the custom data marshalling. */
- private final @Nullable ClassLoader cstDataMarshallClsLdr;
+ private final @NotNull ClassLoader cstDataMarshallClsLdr;
/**
* @param cstDataMarshall Custom data marshaller.
* @param cstDataMarshallClsLdr Class loader for the custom data
marshalling.
*/
- public DiscoveryMessageFactory(@Nullable Marshaller cstDataMarshall,
@Nullable ClassLoader cstDataMarshallClsLdr) {
- assert cstDataMarshall == null && cstDataMarshallClsLdr == null ||
cstDataMarshall != null && cstDataMarshallClsLdr != null;
-
+ public DiscoveryMessageFactory(@NotNull Marshaller cstDataMarshall,
@NotNull ClassLoader cstDataMarshallClsLdr) {
Review Comment:
Still has @NotNull. Other places too
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryClientReconnectMessage.java:
##########
@@ -130,29 +129,15 @@ public boolean success() {
}
/** {@inheritDoc} */
- @Override public void prepareMarshal(Marshaller marsh) {
- if (msgs != null && msgsBytes == null) {
- try {
- msgsBytes = U.marshal(marsh, msgs);
- }
- catch (IgniteCheckedException e) {
- throw new IgniteException("Failed to marshal the pending
messages.", e);
- }
- }
+ @Override public void prepareMarshal(Marshaller marsh) throws
IgniteCheckedException {
+ if (msgs != null)
+ msgsBytes = U.marshal(marsh, msgs);
}
/** {@inheritDoc} */
- @Override public void finishUnmarshal(Marshaller marsh, ClassLoader
clsLdr) {
- if (msgsBytes != null && msgs == null) {
- try {
- msgs = U.unmarshal(marsh, msgsBytes, clsLdr);
-
- msgsBytes = null;
- }
- catch (IgniteCheckedException e) {
- throw new IgniteException("Failed to unmarshal the pending
messages.", e);
- }
- }
+ @Override public void finishUnmarshal(Marshaller marsh, ClassLoader
clsLdr) throws IgniteCheckedException {
+ if (msgsBytes != null)
+ msgs = U.unmarshal(marsh, msgsBytes, clsLdr);
Review Comment:
Let's do not keep `msgsBytes` any more.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/GridIoManager.java:
##########
@@ -449,7 +449,7 @@ public void resetMetrics() {
List<MessageFactoryProvider> compMsgs = new ArrayList<>();
- compMsgs.add(new GridIoMessageFactory());
+ compMsgs.add(new GridIoMessageFactory(marsh, U.gridClassLoader()));
Review Comment:
Why not `ctx.marshallerContext().jdkMarshaller()` ? Is widely used.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/ErrorMessage.java:
##########
@@ -62,61 +47,32 @@ public ErrorMessage() {
}
/**
- * @param err Original error. Will be lazily serialized.
+ * @param err Original error.
*/
public ErrorMessage(@Nullable Throwable err) {
this.err = err;
}
- /**
- * Provides serialized bytes of the error. Should be called only once.
- *
- * @return Serialized error.
- * @see MessageWriter
- */
- public @Nullable byte[] errorBytes() {
- if (err == null)
- return null;
-
+ /** {@inheritDoc} */
+ @Override public void prepareMarshal(Marshaller marsh) throws
IgniteCheckedException {
try {
- return U.marshal(jdk(), err);
+ if (err != null)
+ errBytes = U.marshal(marsh, err);
}
- catch (IgniteCheckedException e0) {
+ catch (IgniteCheckedException e) {
IgniteCheckedException wrappedErr = new
IgniteCheckedException(err.getMessage());
wrappedErr.setStackTrace(err.getStackTrace());
- wrappedErr.addSuppressed(e0);
+ wrappedErr.addSuppressed(e);
- try {
- return U.marshal(jdk(), wrappedErr);
- }
- catch (IgniteCheckedException e1) {
- IgniteException marshErr = new IgniteException("Unable to
marshal the wrapping error.", e1);
-
- marshErr.addSuppressed(wrappedErr);
-
- throw marshErr;
- }
+ errBytes = U.marshal(marsh, wrappedErr);
Review Comment:
no need to keep `errBytes` any more. Less memory consumption. The same in
the other places
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/CalciteErrorMessage.java:
##########
@@ -71,4 +87,16 @@ public long fragmentId() {
@Override public short directType() {
return MessageType.QUERY_ERROR_MESSAGE.directType();
}
+
+ /** {@inheritDoc} */
+ @Override public void prepareMarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
+ if (err != null)
Review Comment:
Usually, we do `&& errBytes == null`
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageSerializerGenerator.java:
##########
@@ -280,9 +280,29 @@ private void start(Collection<String> code, boolean write)
{
returnFalseIfWriteFailed(code, "writer.writeHeader",
"directType()");
if (write && marshallableMessage()) {
+ imports.add("org.apache.ignite.IgniteCheckedException");
+ imports.add("org.apache.ignite.IgniteException");
+
code.add(EMPTY);
+ code.add(identedLine("try {"));
+
+ indent++;
+
code.add(identedLine("msg.prepareMarshal(marshaller);"));
+
+ indent--;
+
+ code.add(identedLine("}"));
+ code.add(identedLine("catch (IgniteCheckedException e) {"));
+
+ indent++;
+
+ code.add(identedLine("throw new IgniteException(\"Failed to
marshal object\", e);"));
Review Comment:
` marshal object\" + msg.getClass().getSimplename()` ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/CalciteErrorMessage.java:
##########
@@ -71,4 +87,16 @@ public long fragmentId() {
@Override public short directType() {
return MessageType.QUERY_ERROR_MESSAGE.directType();
}
+
+ /** {@inheritDoc} */
+ @Override public void prepareMarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
+ if (err != null)
+ errBytes = U.marshal(ctx.marshaller(), err);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void prepareUnmarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
+ if (errBytes != null)
+ err = U.unmarshal(ctx.marshaller(), errBytes,
U.resolveClassLoader(ctx.cache().context().gridConfig()));
Review Comment:
no keed to keep the bytes any more. Less memory consumption
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageSerializerGenerator.java:
##########
@@ -949,8 +969,28 @@ private void finish(List<String> code, boolean read,
boolean marshallable) {
code.add(EMPTY);
if (read && marshallable) {
+ imports.add("org.apache.ignite.IgniteCheckedException");
+ imports.add("org.apache.ignite.IgniteException");
+
+ code.add(identedLine("try {"));
+
+ indent++;
+
code.add(identedLine("msg.finishUnmarshal(marshaller, clsLdr);"));
+ indent--;
+
+ code.add(identedLine("}"));
+ code.add(identedLine("catch (IgniteCheckedException e) {"));
+
+ indent++;
+
+ code.add(identedLine("throw new IgniteException(\"Failed to
unmarshal object\", e);"));
Review Comment:
` unmarshal object\" + msg.getClass().getSimplename()` ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/CalciteErrorMessage.java:
##########
@@ -71,4 +87,16 @@ public long fragmentId() {
@Override public short directType() {
return MessageType.QUERY_ERROR_MESSAGE.directType();
}
+
+ /** {@inheritDoc} */
+ @Override public void prepareMarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
+ if (err != null)
+ errBytes = U.marshal(ctx.marshaller(), err);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void prepareUnmarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
+ if (errBytes != null)
Review Comment:
Usually w no ` && err == null`
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/ErrorMessage.java:
##########
@@ -125,7 +81,7 @@ public void errorBytes(@Nullable byte[] errBytes) {
}
/**
- * Safely gets original error from an error message.
+ * Error.
Review Comment:
Lets just keep @return
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/ErrorMessage.java:
##########
@@ -62,61 +47,32 @@ public ErrorMessage() {
}
/**
- * @param err Original error. Will be lazily serialized.
+ * @param err Original error.
*/
public ErrorMessage(@Nullable Throwable err) {
this.err = err;
}
- /**
- * Provides serialized bytes of the error. Should be called only once.
- *
- * @return Serialized error.
- * @see MessageWriter
- */
- public @Nullable byte[] errorBytes() {
- if (err == null)
- return null;
-
+ /** {@inheritDoc} */
+ @Override public void prepareMarshal(Marshaller marsh) throws
IgniteCheckedException {
try {
- return U.marshal(jdk(), err);
+ if (err != null)
+ errBytes = U.marshal(marsh, err);
}
- catch (IgniteCheckedException e0) {
+ catch (IgniteCheckedException e) {
IgniteCheckedException wrappedErr = new
IgniteCheckedException(err.getMessage());
wrappedErr.setStackTrace(err.getStackTrace());
- wrappedErr.addSuppressed(e0);
+ wrappedErr.addSuppressed(e);
- try {
- return U.marshal(jdk(), wrappedErr);
- }
- catch (IgniteCheckedException e1) {
- IgniteException marshErr = new IgniteException("Unable to
marshal the wrapping error.", e1);
-
- marshErr.addSuppressed(wrappedErr);
-
- throw marshErr;
- }
+ errBytes = U.marshal(marsh, wrappedErr);
}
}
- /**
- * Deserializes the error from {@code errBytes}. Should be called only
once.
- *
- * @param errBytes Serialized error.
- * @see MessageWriter
- */
- public void errorBytes(@Nullable byte[] errBytes) {
- if (F.isEmpty(errBytes))
- err = null;
- else {
- try {
- err = U.unmarshal(jdk(), errBytes, U.gridClassLoader());
- }
- catch (IgniteCheckedException e) {
- throw new IgniteException("Failed to unmarshal error data
bytes.", e);
- }
- }
+ /** {@inheritDoc} */
+ @Override public void finishUnmarshal(Marshaller marsh, ClassLoader
clsLdr) throws IgniteCheckedException {
+ if (errBytes != null)
Review Comment:
Usually we do && err != null. The same in the other places
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/ErrorMessage.java:
##########
@@ -62,61 +47,32 @@ public ErrorMessage() {
}
/**
- * @param err Original error. Will be lazily serialized.
+ * @param err Original error.
*/
public ErrorMessage(@Nullable Throwable err) {
this.err = err;
}
- /**
- * Provides serialized bytes of the error. Should be called only once.
- *
- * @return Serialized error.
- * @see MessageWriter
- */
- public @Nullable byte[] errorBytes() {
- if (err == null)
- return null;
-
+ /** {@inheritDoc} */
+ @Override public void prepareMarshal(Marshaller marsh) throws
IgniteCheckedException {
try {
- return U.marshal(jdk(), err);
+ if (err != null)
Review Comment:
Usually we do `&& errBytes != null`
--
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]
