anton-vinogradov commented on code in PR #13095:
URL: https://github.com/apache/ignite/pull/13095#discussion_r3664568089
##########
modules/core/src/main/java/org/apache/ignite/internal/plugin/AbstractMarshallableMessageFactoryProvider.java:
##########
@@ -18,78 +18,175 @@
package org.apache.ignite.internal.plugin;
import java.lang.reflect.Constructor;
+import java.util.function.Supplier;
import org.apache.ignite.IgniteException;
-import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.MarshallableMessage;
import org.apache.ignite.internal.binary.BinaryMarshaller;
-import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.managers.communication.IgniteMessageFactory;
+import org.apache.ignite.internal.processors.cache.GridCacheMessage;
+import org.apache.ignite.internal.processors.cache.GridCacheMessageDeployer;
import org.apache.ignite.marshaller.Marshaller;
import org.apache.ignite.marshaller.jdk.JdkMarshaller;
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.MessageMarshaller;
import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import
org.apache.ignite.plugin.extensions.communication.NonMarshallableMessage;
+import org.jetbrains.annotations.Nullable;
/**
* An extension of {@link MessageFactoryProvider} allowing to use provided
schema-aware marshaller
- * and resolved class loader to register {@link MarshallableMessage}.
+ * to register {@link MarshallableMessage}.
*/
public abstract class AbstractMarshallableMessageFactoryProvider implements
MessageFactoryProvider {
+ /** Generated-companion constructors per message class, including cached
negative lookups. */
+ private static final ClassValue<Companions> COMPANIONS = new
ClassValue<>() {
+ @Override protected Companions computeValue(Class<?> cls) {
+ return new Companions(companionCtor(cls, "Serializer"),
companionCtor(cls, "Marshaller"), companionCtor(cls, "Deployer"));
+ }
+ };
+
/** Default schema-less marshaller. */
protected Marshaller dfltMarsh;
- /** Default class loader. */
- protected final ClassLoader dftlClsLdr = U.gridClassLoader();
-
/** Schema-aware marshaller like {@link BinaryMarshaller}. */
protected Marshaller schemaAwareMarsh;
- /** Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}. */
- protected ClassLoader resolvedClsLdr;
-
/**
* @param dfltMarsh Default schema-less marshaller like {@link
JdkMarshaller}.
* @param schemaAwareMarsh Schema-aware marshaller like {@link
BinaryMarshaller}.
- * @param resolvedClsLdr Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}.
*/
- public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh,
ClassLoader resolvedClsLdr) {
+ public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh) {
this.dfltMarsh = dfltMarsh;
this.schemaAwareMarsh = schemaAwareMarsh;
- this.resolvedClsLdr = resolvedClsLdr;
}
- /** Registers message automatically generating message supplier and
serializer. */
- protected static <T extends Message> void register(MessageFactory factory,
Class<T> cls, short id, Marshaller marsh,
- ClassLoader clsLrd) {
+ /** Registers a message with its generated serializer, marshaller (if
marshallable), and deployer (if any). */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id, Marshaller marsh) {
Constructor<T> ctor;
- MessageSerializer<T> serializer;
try {
ctor = cls.getConstructor();
+ }
+ catch (NoSuchMethodException e) {
+ throw new IgniteException("Failed to register message of type " +
cls.getSimpleName(), e);
+ }
+
+ register(factory, cls, id, () -> {
+ try {
+ return ctor.newInstance();
+ }
+ catch (Exception e) {
+ throw new IgniteException("Failed to create message of type "
+ cls.getSimpleName(), e);
+ }
+ }, marsh);
+ }
+
+ /**
+ * Registers a message with a caller-provided {@code supplier} and its
generated serializer, marshaller (if
+ * marshallable), and deployer (if any). Use this overload when {@code
cls} is package-private and so cannot be
+ * instantiated by reflection from this package — pass an in-package
{@code ::new} reference as {@code supplier}.
+ */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id,
+ Supplier<Message> supplier, Marshaller marsh) {
+ MessageSerializer<T> serializer = requireGenerated(cls, "Serializer",
marsh);
+
+ // A MarshallableMessage always gets a generated marshaller (the hook
call alone is a statement), so its
+ // absence is a build problem. For the rest the generator skips
statement-free marshallers, so absence
+ // legitimately means "nothing to marshal"; the message and its
companions ship in the same jar, hence
+ // a missing class cannot be a packaging accident that spares the
(required) serializer.
+ MessageMarshaller<T> marshaller =
NonMarshallableMessage.class.isAssignableFrom(cls)
+ ? null
Review Comment:
Rewritten as an if/else chain — the three cases (NonMarshallableMessage /
MarshallableMessage / the rest) now read one per branch instead of nested
ternaries.
##########
modules/core/src/main/java/org/apache/ignite/internal/plugin/AbstractMarshallableMessageFactoryProvider.java:
##########
@@ -18,78 +18,175 @@
package org.apache.ignite.internal.plugin;
import java.lang.reflect.Constructor;
+import java.util.function.Supplier;
import org.apache.ignite.IgniteException;
-import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.MarshallableMessage;
import org.apache.ignite.internal.binary.BinaryMarshaller;
-import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.managers.communication.IgniteMessageFactory;
+import org.apache.ignite.internal.processors.cache.GridCacheMessage;
+import org.apache.ignite.internal.processors.cache.GridCacheMessageDeployer;
import org.apache.ignite.marshaller.Marshaller;
import org.apache.ignite.marshaller.jdk.JdkMarshaller;
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.MessageMarshaller;
import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import
org.apache.ignite.plugin.extensions.communication.NonMarshallableMessage;
+import org.jetbrains.annotations.Nullable;
/**
* An extension of {@link MessageFactoryProvider} allowing to use provided
schema-aware marshaller
- * and resolved class loader to register {@link MarshallableMessage}.
+ * to register {@link MarshallableMessage}.
*/
public abstract class AbstractMarshallableMessageFactoryProvider implements
MessageFactoryProvider {
+ /** Generated-companion constructors per message class, including cached
negative lookups. */
+ private static final ClassValue<Companions> COMPANIONS = new
ClassValue<>() {
+ @Override protected Companions computeValue(Class<?> cls) {
+ return new Companions(companionCtor(cls, "Serializer"),
companionCtor(cls, "Marshaller"), companionCtor(cls, "Deployer"));
+ }
+ };
+
/** Default schema-less marshaller. */
protected Marshaller dfltMarsh;
- /** Default class loader. */
- protected final ClassLoader dftlClsLdr = U.gridClassLoader();
-
/** Schema-aware marshaller like {@link BinaryMarshaller}. */
protected Marshaller schemaAwareMarsh;
- /** Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}. */
- protected ClassLoader resolvedClsLdr;
-
/**
* @param dfltMarsh Default schema-less marshaller like {@link
JdkMarshaller}.
* @param schemaAwareMarsh Schema-aware marshaller like {@link
BinaryMarshaller}.
- * @param resolvedClsLdr Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}.
*/
- public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh,
ClassLoader resolvedClsLdr) {
+ public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh) {
this.dfltMarsh = dfltMarsh;
this.schemaAwareMarsh = schemaAwareMarsh;
- this.resolvedClsLdr = resolvedClsLdr;
}
- /** Registers message automatically generating message supplier and
serializer. */
- protected static <T extends Message> void register(MessageFactory factory,
Class<T> cls, short id, Marshaller marsh,
- ClassLoader clsLrd) {
+ /** Registers a message with its generated serializer, marshaller (if
marshallable), and deployer (if any). */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id, Marshaller marsh) {
Constructor<T> ctor;
- MessageSerializer<T> serializer;
try {
ctor = cls.getConstructor();
+ }
+ catch (NoSuchMethodException e) {
+ throw new IgniteException("Failed to register message of type " +
cls.getSimpleName(), e);
+ }
+
+ register(factory, cls, id, () -> {
+ try {
+ return ctor.newInstance();
+ }
+ catch (Exception e) {
+ throw new IgniteException("Failed to create message of type "
+ cls.getSimpleName(), e);
+ }
+ }, marsh);
+ }
+
+ /**
+ * Registers a message with a caller-provided {@code supplier} and its
generated serializer, marshaller (if
+ * marshallable), and deployer (if any). Use this overload when {@code
cls} is package-private and so cannot be
+ * instantiated by reflection from this package — pass an in-package
{@code ::new} reference as {@code supplier}.
+ */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id,
+ Supplier<Message> supplier, Marshaller marsh) {
+ MessageSerializer<T> serializer = requireGenerated(cls, "Serializer",
marsh);
+
+ // A MarshallableMessage always gets a generated marshaller (the hook
call alone is a statement), so its
+ // absence is a build problem. For the rest the generator skips
statement-free marshallers, so absence
+ // legitimately means "nothing to marshal"; the message and its
companions ship in the same jar, hence
+ // a missing class cannot be a packaging accident that spares the
(required) serializer.
+ MessageMarshaller<T> marshaller =
NonMarshallableMessage.class.isAssignableFrom(cls)
+ ? null
+ : MarshallableMessage.class.isAssignableFrom(cls)
+ ? requireGenerated(cls, "Marshaller", marsh)
+ : loadGenerated(cls, "Marshaller", marsh);
+
+ // Deployers are generated for GridCacheMessage subclasses only, so
the class lookup is skipped for the rest;
+ // a DeployableMessage left without a deployer is then rejected at
registration.
+ GridCacheMessageDeployer deployer =
GridCacheMessage.class.isAssignableFrom(cls)
Review Comment:
Done — `GridCacheMessageDeployer<?>`; it was the only raw use left there.
##########
modules/core/src/main/java/org/apache/ignite/spi/communication/tcp/internal/GridNioServerWrapper.java:
##########
@@ -834,18 +851,18 @@ private MessageFactory get() {
};
GridNioMessageReaderFactory readerFactory = new
GridNioMessageReaderFactory() {
- private IgniteSpiContext context;
+ private IgniteSpiContext spiCtx;
private MessageFormatter formatter;
- @Override public MessageReader reader(GridNioSession ses,
MessageFactory msgFactory)
+ @Override public MessageReader reader(GridNioSession ses,
MessageFactory<? extends Message> msgFactory)
throws IgniteCheckedException {
final IgniteSpiContext ctx =
stateProvider.getSpiContextWithoutInitialLatch();
- if (formatter == null || context != ctx) {
- context = ctx;
+ if (formatter == null || this.spiCtx != ctx) {
Review Comment:
Removed — both occurrences; `formatter` right next to them was already
unqualified.
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageProcessorTest.java:
##########
@@ -352,6 +401,154 @@ public void
testCompressAnnotationFailsForUnsupportedTypes() {
assertThat(compilation).hadErrorContaining("Compress annotation is
used for an unsupported type: java.util.List");
}
+ /** Verifies that {@code @NioField} on a non-{@link Message}-typed field
is a compilation error. */
+ @Test
+ public void testNioFieldOnNonMessageTypeFails() {
+ Compilation compilation = compile("NioFieldOnNonMessageMessage.java");
+
+ assertThat(compilation).failed();
+ assertThat(compilation).hadErrorContaining("@NioField has no effect on
non-Message field");
+ }
+
+ /** Verifies that {@code @NioField} on a message needing a cache object
context is a compilation error. */
+ @Test
+ public void testNioFieldNeedingCacheContextFails() {
+ Compilation compilation = compile("NioFieldNeedsCtxMessage.java");
+
+ assertThat(compilation).failed();
+ assertThat(compilation).hadErrorContaining("needs a cache object
context to unmarshal");
+ }
+
+ /** Verifies that {@code @Marshalled} generates {@code U.unmarshal} with a
blank line before the null-out. */
+ @Test
+ public void testMarshalledMessage() {
+ Compilation compilation = compile("TestMarshalledMessage.java");
+
+ assertThat(compilation).succeeded();
+
+ assertEquals(2, compilation.generatedSourceFiles().size());
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledMessageSerializer")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledMessageSerializer.java"));
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledMessageMarshaller")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledMessageMarshaller.java"));
+ }
+
+ /** Verifies that {@code @Marshalled} generates Set reconstruction in
FINISH_CACHE mode. */
+ @Test
+ public void testMarshalledCollectionMessage() {
+ Compilation compilation =
compile("TestMarshalledCollectionMessage.java");
+
+ assertThat(compilation).succeeded();
+
+ assertEquals(2, compilation.generatedSourceFiles().size());
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledCollectionMessageSerializer")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledCollectionMessageSerializer.java"));
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledCollectionMessageMarshaller")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledCollectionMessageMarshaller.java"));
+ }
+
+ /** Verifies that {@code @Marshalled} generates Map reconstruction in
FINISH_CACHE mode. */
Review Comment:
You're right, that javadoc was stale: there is no FINISH_CACHE mode in the
code any more (the phrase survived from an earlier iteration of the generator).
The test simply compares the generated companions against the goldens for a
map-shaped @Marshalled field, so the javadoc now says exactly that — same for
the Set one above it.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageCompanionGenerator.java:
##########
@@ -0,0 +1,289 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.PrintWriter;
+import java.io.Reader;
+import java.io.Writer;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+import javax.annotation.processing.FilerException;
+import javax.annotation.processing.ProcessingEnvironment;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
+import javax.lang.model.type.TypeMirror;
+import javax.lang.model.util.ElementFilter;
+import javax.lang.model.util.Elements;
+import javax.tools.Diagnostic;
+import javax.tools.FileObject;
+import javax.tools.JavaFileObject;
+import javax.tools.StandardLocation;
+import
org.apache.ignite.internal.systemview.SystemViewRowAttributeWalkerProcessor;
+
+/**
+ * Base class for message code generators ({@link MessageSerializerGenerator},
{@link MessageMarshallerGenerator},
+ * {@link MessageDeploymentGenerator}).
+ */
+public abstract class MessageGenerator {
+ /** Blank separator line in generated code. */
+ public static final String EMPTY = "";
+
+ /** Platform line separator used in generated code. */
+ public static final String NL = System.lineSeparator();
+
+ /** Single indentation unit. */
+ public static final String TAB = " ";
+
+ /** Javadoc stub emitted on every generated {@code @Override} method. */
+ public static final String METHOD_JAVADOC = "/** */";
+
+ /** Javadoc block emitted on every generated class. */
+ public static final String CLS_JAVADOC = "/**" + NL +
+ " * This class is generated automatically." + NL +
+ " *" + NL +
+ " * @see " + MessageProcessor.class.getName() + NL +
+ " */";
+
+ /** */
+ protected final ProcessingEnvironment env;
+
+ /** */
+ protected final Set<String> imports = new TreeSet<>();
+
+ /** */
+ protected TypeElement type;
+
+ /** Current indentation level. Set to the class-member level once in
{@link #generate}; adjusted only by balanced shifts. */
+ protected int indent;
+
+ /** Dispatches cache-object-context resolution in both the marshaller and
the deployment generators. */
+ protected final TypeMirror cacheIdAwareType;
+
+ /** */
+ MessageGenerator(ProcessingEnvironment env) {
+ this.env = env;
+
+ cacheIdAwareType =
type("org.apache.ignite.plugin.extensions.communication.CacheIdAware");
+ }
+
+ /** */
+ boolean isCacheIdAwareMessage(TypeElement te) {
+ return assignableFrom(te.asType(), cacheIdAwareType);
+ }
+
+ /** Generates and writes the source file for {@code type}; skipped when
{@link #shouldSkip} returns {@code true}. */
+ final void generate(TypeElement type, List<VariableElement> fields) throws
Exception {
+ assert this.type == null : "Message" + typeSuffix() + " generator
isn't stateless and is supposed to be single-use.";
+
+ if (shouldSkip(type, fields))
+ return;
+
+ this.type = type;
+
+ indent = 1;
+
+ generateBody(fields);
+
+ String clsName = type.getSimpleName() + typeSuffix();
+ String fqnClsName = env.getElementUtils().getPackageOf(type) + "." +
clsName;
+ String code = buildClassCode(clsName);
+
+ if (code == null)
+ return;
+
+ try {
+ JavaFileObject file = env.getFiler().createSourceFile(fqnClsName);
+
+ try (Writer writer = file.openWriter()) {
+ writer.append(code);
+ writer.flush();
+ }
+ }
+ catch (FilerException e) {
+ if (!identicalFileIsAlreadyGenerated(env, code, fqnClsName)) {
+ env.getMessager().printMessage(Diagnostic.Kind.ERROR,
+ "Message" + typeSuffix() + " " + clsName + " is already
generated. Try 'mvn clean install' to fix the issue.");
+
+ throw e;
+ }
+ }
+ }
+
+ /** @return Class name suffix: {@code "Serializer"} or {@code
"Marshaller"}. */
+ abstract String typeSuffix();
+
+ /** @return {@code true} if no file should be generated for this type;
default is {@code false}. */
+ boolean shouldSkip(TypeElement type, List<VariableElement> fields) {
Review Comment:
Agreed — the members meant for subclasses are `protected` now (typeSuffix,
shouldSkip, generateBody, buildClassCode, generate, writeClassHeader,
indentedLine, simpleNameWithGeneric, assignableFrom, type, erasedType,
fieldAccessor, isCacheIdAwareMessage) together with the overrides in the three
generators. The static helpers stay public — the idto package uses them.
--
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]
