You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/06/01 14:23:40 UTC
[GitHub] [ignite-3] ibessonov commented on a change in pull request #153: IGNITE-14715 Add annotation processor for generating network message implementations
ibessonov commented on a change in pull request #153:
URL: https://github.com/apache/ignite-3/pull/153#discussion_r643120353
##########
File path: modules/network-annotation-processor/src/integrationTest/resources/org/apache/ignite/network/processor/internal/NetworkMessageProcessorMessageTypes.java
##########
@@ -17,8 +17,9 @@
package org.apache.ignite.network.processor.internal;
-public class TransitiveMessageFactory {
- public static TransitiveMessage.Builder transitiveMessage() {
- return new TransitiveMessageImpl();
- }
+import org.apache.ignite.network.annotations.ModuleMessageTypes;
+
+@ModuleMessageTypes(moduleType = 1, moduleName = "NetworkMessageProcessorTest")
Review comment:
I remember Andrey Gura saying that word "module" doesn't fit that well, can we come up with something better? Like scope or group...
##########
File path: modules/network-annotation-processor/src/integrationTest/resources/org/apache/ignite/network/processor/internal/AllTypesMessage.java
##########
@@ -23,12 +23,10 @@
import java.util.UUID;
import org.apache.ignite.lang.IgniteUuid;
import org.apache.ignite.network.NetworkMessage;
-import org.apache.ignite.network.processor.annotations.AutoSerializable;
+import org.apache.ignite.network.annotations.AutoMessage;
-@AutoSerializable(messageFactory = AllTypesMessageFactory.class)
+@AutoMessage(1)
Review comment:
Name is a bit misleading. It's hard to call classes like "AffinityTopologyVersion" messages, you know.
##########
File path: modules/network/src/main/java/org/apache/ignite/network/internal/direct/DirectMarshallingUtils.java
##########
@@ -18,31 +18,33 @@
package org.apache.ignite.network.internal.direct;
import java.nio.ByteBuffer;
+import org.apache.ignite.network.internal.direct.stream.DirectByteBufferStream;
/**
* Direct marshalling utils.
*/
public class DirectMarshallingUtils {
/**
- * Reads a direct message type from a byte buffer.
+ * Reads a {@code short} from a byte buffer in an order defined by the {@link DirectByteBufferStream}
+ * implementation.
*
* @param buffer Byte buffer.
* @return Direct message type.
*/
- public static short getMessageType(ByteBuffer buffer) {
+ public static short getShort(ByteBuffer buffer) {
byte b0 = buffer.get();
Review comment:
I would compress this and assert during write (not only) that type cannot be more then 0x3FFF. We should have compressed shorts somewhere already, don't we?
##########
File path: modules/metastorage-client/src/integrationTest/java/org/apache/ignite/internal/metastorage/client/ITMetaStorageServiceTest.java
##########
@@ -91,16 +85,17 @@
public static final int LATEST_REVISION = -1;
/** Factory. */
- private static final RaftClientMessageFactory FACTORY = new RaftClientMessageFactoryImpl();
+ private static final RaftClientMessagesFactory FACTORY = new RaftClientMessagesFactory();
/** Network factory. */
private static final ClusterServiceFactory NETWORK_FACTORY = new TestScaleCubeClusterServiceFactory();
/** */
- private static final MessageSerializationRegistry SERIALIZATION_REGISTRY = new MessageSerializationRegistry()
- .registerFactory(ScaleCubeMessage.TYPE, new ScaleCubeMessageSerializationFactory())
- .registerFactory(HandshakeStartMessage.TYPE, new HandshakeStartMessageSerializationFactory())
- .registerFactory(HandshakeStartResponseMessage.TYPE, new HandshakeStartResponseMessageSerializationFactory());
+ private static final MessageSerializationRegistry SERIALIZATION_REGISTRY = new MessageSerializationRegistry();
+
+ static {
Review comment:
Should this be done in BeforeAll instead of static initializer?
##########
File path: modules/network/src/main/java/org/apache/ignite/network/internal/recovery/RecoveryClientHandshakeManager.java
##########
@@ -44,23 +44,29 @@
/** Handshake completion future. */
private final CompletableFuture<NettySender> handshakeCompleteFuture = new CompletableFuture<>();
+ /** Message factory. */
+ private final NetworkMessagesFactory messageFactory;
+
/**
* Constructor.
*
* @param launchId Launch id.
* @param consistentId Consistent id.
*/
- public RecoveryClientHandshakeManager(UUID launchId, String consistentId) {
+ public RecoveryClientHandshakeManager(
+ UUID launchId, String consistentId, NetworkMessagesFactory messageFactory
+ ) {
this.launchId = launchId;
this.consistentId = consistentId;
+ this.messageFactory = messageFactory;
}
/** {@inheritDoc} */
@Override public HandshakeAction onMessage(Channel channel, NetworkMessage message) {
if (message instanceof HandshakeStartMessage) {
HandshakeStartMessage msg = (HandshakeStartMessage) message;
- HandshakeStartResponseMessage response = HandshakeMessageFactory.handshakeStartResponseMessage()
+ HandshakeStartResponseMessage response = messageFactory.handshakeStartResponseMessage()
Review comment:
BTW, why can't these methods be static?
##########
File path: modules/network/src/test/java/org/apache/ignite/network/internal/AllTypesMessageGenerator.java
##########
@@ -81,139 +79,142 @@ public static AllTypesMessage generate(long seed, boolean nestedMsg) {
* Generate random value.
*
* @param random Seeded random.
- * @param type Value type.
+ * @param field Field which value is being generated.
* @param nestedMsg {@code true} if nested messages should be generated, {@code false} otherwise.
* @return Random value.
- * @throws Exception If failed.
*/
- private static Object randomValue(Random random, MessageCollectionItemType type, boolean nestedMsg) throws Exception {
- switch (type) {
- case BYTE:
- return (byte) random.nextInt();
-
- case SHORT:
- return (short) random.nextInt();
-
- case INT:
- return random.nextInt();
-
- case LONG:
- return random.nextLong();
-
- case FLOAT:
- return random.nextFloat();
-
- case DOUBLE:
- return random.nextDouble();
-
- case CHAR:
- return (char) random.nextInt();
-
- case BOOLEAN:
- return random.nextBoolean();
-
- case BYTE_ARR:
- int byteArrLen = random.nextInt(1024);
- byte[] bytes = new byte[byteArrLen];
- random.nextBytes(bytes);
- return bytes;
-
- case SHORT_ARR:
- int shortArrLen = random.nextInt(1024);
- short[] shorts = new short[1024];
- for (int i = 0; i < shortArrLen; i++) {
- shorts[i] = (short) random.nextInt();
- }
- return shorts;
-
- case INT_ARR:
- int intArrLen = random.nextInt(1024);
- int[] ints = new int[1024];
- for (int i = 0; i < intArrLen; i++) {
- ints[i] = random.nextInt();
- }
- return ints;
-
- case LONG_ARR:
- int longArrLen = random.nextInt(1024);
- long[] longs = new long[1024];
- for (int i = 0; i < longArrLen; i++) {
- longs[i] = random.nextLong();
- }
- return longs;
+ @Nullable
+ private static Object randomValue(Random random, Field field, boolean nestedMsg) {
+ Class<?> type = field.getType();
- case FLOAT_ARR:
- int floatArrLen = random.nextInt(1024);
- float[] floats = new float[1024];
- for (int i = 0; i < floatArrLen; i++) {
- floats[i] = random.nextFloat();
- }
- return floats;
-
- case DOUBLE_ARR:
- int doubleArrLen = random.nextInt(1024);
- double[] doubles = new double[1024];
- for (int i = 0; i < doubleArrLen; i++) {
- doubles[i] = random.nextDouble();
- }
- return doubles;
-
- case CHAR_ARR:
- int charArrLen = random.nextInt(1024);
- char[] chars = new char[1024];
- for (int i = 0; i < charArrLen; i++) {
- chars[i] = (char) random.nextInt();
- }
- return chars;
-
- case BOOLEAN_ARR:
- int booleanArrLen = random.nextInt(1024);
- boolean[] booleans = new boolean[1024];
- for (int i = 0; i < booleanArrLen; i++) {
- booleans[i] = random.nextBoolean();
- }
- return booleans;
-
- case STRING:
- int aLetter = 'a';
- int strLen = random.nextInt(1024);
- StringBuilder sb = new StringBuilder();
- for (int i = 0; i < strLen; i++) {
- int letter = aLetter + random.nextInt(26);
- sb.append(letter);
- }
- return sb.toString();
-
- case BIT_SET:
- BitSet set = new BitSet();
- int setLen = random.nextInt(10);
- for (int i = 0; i < setLen; i++) {
- if (random.nextBoolean()) {
- set.set(i);
- }
- }
-
- return set;
-
- case UUID:
- byte[] uuidBytes = new byte[16];
- random.nextBytes(uuidBytes);
- return UUID.nameUUIDFromBytes(uuidBytes);
+ if (type == byte.class) {
+ return (byte) random.nextInt();
+ }
Review comment:
Too many braces
##########
File path: modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/RaftCounterServerAbstractTest.java
##########
@@ -51,10 +45,11 @@
protected static final int PORT = 20010;
/** */
- private static final MessageSerializationRegistry SERIALIZATION_REGISTRY = new MessageSerializationRegistry()
- .registerFactory(ScaleCubeMessage.TYPE, new ScaleCubeMessageSerializationFactory())
- .registerFactory(HandshakeStartMessage.TYPE, new HandshakeStartMessageSerializationFactory())
- .registerFactory(HandshakeStartResponseMessage.TYPE, new HandshakeStartResponseMessageSerializationFactory());
+ private static final MessageSerializationRegistry SERIALIZATION_REGISTRY = new MessageSerializationRegistry();
+
+ static {
+ NetworkMessagesSerializationRegistryInitializer.initialize(SERIALIZATION_REGISTRY);
Review comment:
Should this be by default in every registry?
##########
File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/network/processor/internal/NetworkMessageProcessor.java
##########
@@ -0,0 +1,254 @@
+/*
+ * 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.network.processor.internal;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import javax.annotation.processing.AbstractProcessor;
+import javax.annotation.processing.RoundEnvironment;
+import javax.lang.model.SourceVersion;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.ElementKind;
+import javax.lang.model.element.TypeElement;
+import javax.tools.Diagnostic;
+import com.squareup.javapoet.JavaFile;
+import com.squareup.javapoet.TypeSpec;
+import org.apache.ignite.network.NetworkMessage;
+import org.apache.ignite.network.annotations.AutoMessage;
+import org.apache.ignite.network.annotations.ModuleMessageTypes;
+import org.apache.ignite.network.processor.internal.messages.MessageBuilderGenerator;
+import org.apache.ignite.network.processor.internal.messages.MessageFactoryGenerator;
+import org.apache.ignite.network.processor.internal.messages.MessageImplGenerator;
+import org.apache.ignite.network.processor.internal.serialization.MessageDeserializerGenerator;
+import org.apache.ignite.network.processor.internal.serialization.MessageSerializerGenerator;
+import org.apache.ignite.network.processor.internal.serialization.RegistryInitializerGenerator;
+import org.apache.ignite.network.processor.internal.serialization.SerializationFactoryGenerator;
+import org.apache.ignite.network.serialization.MessageDeserializer;
+import org.apache.ignite.network.serialization.MessageSerializationFactory;
+import org.apache.ignite.network.serialization.MessageSerializationRegistry;
+import org.apache.ignite.network.serialization.MessageSerializer;
+
+/**
+ * Annotation processor for working with the {@link AutoMessage} annotation.
+ */
+public class NetworkMessageProcessor extends AbstractProcessor {
+ /** {@inheritDoc} */
+ @Override public Set<String> getSupportedAnnotationTypes() {
+ return Set.of(AutoMessage.class.getName());
+ }
+
+ /** {@inheritDoc} */
+ @Override public SourceVersion getSupportedSourceVersion() {
+ return SourceVersion.latest();
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
+ List<MessageClass> messages = annotations.stream()
+ .map(roundEnv::getElementsAnnotatedWith)
+ .flatMap(Collection::stream)
+ .map(TypeElement.class::cast)
+ .map(MessageClass::new)
+ .collect(Collectors.toList());
+
+ if (messages.isEmpty()) {
+ return true;
+ }
+
+ try {
+ validateMessages(messages);
+
+ MessageTypes moduleMessageTypes = getModuleMessageTypes(roundEnv);
+
+ generateMessageImpls(messages, moduleMessageTypes);
+
+ generateSerializers(messages, moduleMessageTypes);
+ }
+ catch (ProcessingException e) {
+ processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR, e.getMessage(), e.getElement());
+ }
+
+ return true;
+ }
+
+ /**
+ * Generates the following classes for the current compilation unit:
+ *
+ * <ol>
+ * <li>Builder interfaces;</li>
+ * <li>Network Message and Builder implementations;</li>
+ * <li>Message factory for all generated messages.</li>
+ * </ol>
+ */
+ private void generateMessageImpls(List<MessageClass> annotatedMessages, MessageTypes messageTypes) {
+ for (MessageClass message : annotatedMessages) {
+ try {
+ // generate a Builder interface with setters
+ TypeSpec builder = new MessageBuilderGenerator(processingEnv, message)
+ .generateBuilderInterface();
+
+ writeToFile(message.packageName(), builder);
+
+ // generate the message and the builder implementations
+ TypeSpec messageImpl = new MessageImplGenerator(processingEnv, message, builder, messageTypes)
+ .generateMessageImpl();
+
+ writeToFile(message.packageName(), messageImpl);
+ }
+ catch (ProcessingException e) {
+ throw new ProcessingException(e.getMessage(), e.getCause(), message.element());
+ }
+ }
+
+ // generate a factory for all messages inside the current compilation unit
+ TypeSpec messageFactory = new MessageFactoryGenerator(processingEnv, annotatedMessages, messageTypes)
Review comment:
I suspect that this won't work for incremental messages changes.
##########
File path: modules/network-api/src/main/java/org/apache/ignite/network/annotations/AutoMessage.java
##########
@@ -32,41 +30,65 @@
import org.apache.ignite.network.serialization.MessageSerializer;
/**
- * Annotation for marking network message interfaces (i.e. those extending the {@link NetworkMessage} interface). For
- * such interfaces, the following classes will be generated:
+ * Annotation for interfaces intended to be used as <i>Network Messages</i>. A Network Message is an interface that
+ * must satisfy the following requirements:
+ *
+ * <ol>
+ * <li>It must extend the {@link NetworkMessage} interface either directly or transitively;</li>
+ * <li>It must only contain Ignite-style getter methods that represent the message's properties.</li>
+ * </ol>
+ *
+ * When such interface is marked by this annotation, it can be used by the annotation processor to generate
+ * the following classes:
+ *
+ * <ol>
+ * <li>Builder interface with setters for all declared properties;</li>
+ * <li>An immutable implementation of the message and a nested implementation of the generated builder
+ * for creating new message instances;</li>
+ * </ol>
+ *
+ * If the {@link #autoSerializable} property is set to {@code true}, the annotation processor will additionally generate
+ * serialization-related classes:
*
* <ol>
* <li>{@link MessageSerializer};</li>
* <li>{@link MessageDeserializer};</li>
* <li>{@link MessageSerializationFactory}.</li>
* </ol>
- * <p>
- * These messages must obey the <i>network message declaration contract</i> and can only contain
- * <i>directly marshallable types</i>, which can be one of the following:
+ *
+ * Properties of messages that can be auto-serialized can only be of <i>directly marshallable type</i>,
+ * which is one of the following:
*
* <ol>
* <li>Primitive type;</li>
- * <li>{@link String};</li>
+ * <li>{@code String};</li>
* <li>{@link UUID};</li>
* <li>{@link IgniteUuid};</li>
* <li>{@link BitSet};</li>
- * <li>Nested {@link NetworkMessage};</li>
+ * <li>Nested {@code NetworkMessage};</li>
* <li>Array of primitive types, corresponding boxed types or other directly marshallable types;</li>
- * <li>{@link Collection} of boxed primitive types or other directly marshallable types;</li>
- * <li>{@link Map} where both keys and values can be of a directly marshallable type.</li>
+ * <li>{@code Collection} of boxed primitive types or other directly marshallable types;</li>
+ * <li>{@code Map} where both keys and values can be of a directly marshallable type.</li>
* </ol>
+ *
+ * After all messages in the module have been processed, the processor will use the <i>module descriptor</i> (class
+ * annotated with {@link ModuleMessageTypes}) to expose the builders via a
+ * Message Factory.
+ *
+ * @see ModuleMessageTypes
*/
-// TODO: describe the message declaration contract, see https://issues.apache.org/jira/browse/IGNITE-14715
@Target(ElementType.TYPE)
// using the RUNTIME retention policy in order to avoid problems with incremental compilation in an IDE.
@Retention(RetentionPolicy.RUNTIME)
-public @interface AutoSerializable {
+public @interface AutoMessage {
+ /**
+ * This message's type as described in {@link NetworkMessage#messageType}.
+ */
+ short value();
+
/**
- * Message factory class that will be used to create message builders during deserialization.
- * <p>
- * Message factories must have a static method with the same name as the created message type and return a builder
- * for that type, e.g. a factory for creating {@code TestMessage} instances must have a {@code TestMessage.Builder
- * testMessage()} method.
+ * When this property is set to {@code true} (default), serialization-related classes will be generated in addition
+ * to the message implementation.
*/
- Class<?> messageFactory();
+ boolean autoSerializable() default true;
Review comment:
Why do we need this?
##########
File path: modules/network-annotation-processor/src/integrationTest/resources/org/apache/ignite/network/processor/internal/ConflictingTypeMessage.java
##########
@@ -17,11 +17,11 @@
package org.apache.ignite.network.processor.internal;
-import java.util.Collection;
+import java.util.ArrayList;
import org.apache.ignite.network.NetworkMessage;
-import org.apache.ignite.network.processor.annotations.AutoSerializable;
+import org.apache.ignite.network.annotations.AutoMessage;
-@AutoSerializable(messageFactory = AllTypesMessageFactory.class)
-public interface MissingBuilderMessage extends NetworkMessage {
+@AutoMessage(1)
+public interface ConflictingTypeMessage extends NetworkMessage {
Review comment:
All classes should extend NetworkMessage, right? I suggest generifying it.
##########
File path: modules/network-api/src/main/java/org/apache/ignite/network/serialization/MessageSerializationRegistry.java
##########
@@ -25,22 +27,29 @@
*/
public class MessageSerializationRegistry {
/** message type -> MessageSerializerProvider instance */
- private final MessageSerializationFactory<?>[] factories = new MessageSerializationFactory<?>[Short.MAX_VALUE << 1];
+ private final Map<Integer, MessageSerializationFactory<?>> factories = new HashMap<>();
Review comment:
I disagree, this particular structure must be effective.
MessageSerializationFactory<?>[][] would make more sense.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org