sashapolo commented on a change in pull request #714: URL: https://github.com/apache/ignite-3/pull/714#discussion_r829764385
########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/IncrementalCompilationConfig.java ########## @@ -0,0 +1,187 @@ +/* + * 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.network.processor; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.squareup.javapoet.ClassName; +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Reader; +import java.nio.file.NoSuchFileException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import javax.annotation.processing.Filer; +import javax.annotation.processing.ProcessingEnvironment; +import javax.tools.FileObject; +import javax.tools.StandardLocation; + +/** + * Incremental configuration of the {@link TransferableObjectProcessor}. + * Holds data between (re-)compilations. + */ +class IncrementalCompilationConfig { + /** Incremental compilation configuration file name. */ + static final String CONFIG_FILE_NAME = "META-INF/transferable.messages"; + + /** Message group class name. */ + private ClassName messageGroupClassName; + + /** Messages. */ + private final List<ClassName> messageClasses; + + IncrementalCompilationConfig(ClassName messageGroupClassName, List<ClassName> messageClasses) { + this.messageGroupClassName = messageGroupClassName; + this.messageClasses = messageClasses; + } + + /** + * Saves configuration on disk. + * + * @param processingEnv Processing environment. + */ + void writeConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject fileObject; + try { + fileObject = filer.createResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + + try (OutputStream out = fileObject.openOutputStream()) { + BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(out, UTF_8)); + writeClassName(writer, messageGroupClassName); + writer.newLine(); + + for (ClassName messageClassName : messageClasses) { + writeClassName(writer, messageClassName); + writer.newLine(); + } + + writer.flush(); + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Reads configuration from disk. + * + * @param processingEnv Processing environment. + */ + static IncrementalCompilationConfig readConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject resource; + + try { + resource = filer.getResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + return null; + } + + try (Reader reader = resource.openReader(true)) { + BufferedReader bufferedReader = new BufferedReader(reader); + String messageClassNameString = bufferedReader.readLine(); + + if (messageClassNameString == null) { + return null; + } + + ClassName messageClassName = readClassName(messageClassNameString); + + List<ClassName> message = new ArrayList<>(); + + String line; + while ((line = bufferedReader.readLine()) != null) { + ClassName className = readClassName(line); + message.add(className); + } + + return new IncrementalCompilationConfig(messageClassName, message); + } catch (FileNotFoundException | NoSuchFileException e) { + return null; + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Writes class name with all the enclosing classes. + * + * @param writer Writer. + * @param className Class name. + * @throws IOException If failed. + */ + private void writeClassName(BufferedWriter writer, ClassName className) throws IOException { + writer.write(className.packageName()); + writer.write(' '); + + List<String> enclosingSimpleNames = new ArrayList<>(); Review comment: usually your code looks fine in this regard, this is the only place I've found ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/IncrementalCompilationConfig.java ########## @@ -0,0 +1,187 @@ +/* + * 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.network.processor; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.squareup.javapoet.ClassName; +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Reader; +import java.nio.file.NoSuchFileException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import javax.annotation.processing.Filer; +import javax.annotation.processing.ProcessingEnvironment; +import javax.tools.FileObject; +import javax.tools.StandardLocation; + +/** + * Incremental configuration of the {@link TransferableObjectProcessor}. + * Holds data between (re-)compilations. + */ +class IncrementalCompilationConfig { + /** Incremental compilation configuration file name. */ + static final String CONFIG_FILE_NAME = "META-INF/transferable.messages"; + + /** Message group class name. */ + private ClassName messageGroupClassName; + + /** Messages. */ + private final List<ClassName> messageClasses; + + IncrementalCompilationConfig(ClassName messageGroupClassName, List<ClassName> messageClasses) { + this.messageGroupClassName = messageGroupClassName; + this.messageClasses = messageClasses; + } + + /** + * Saves configuration on disk. + * + * @param processingEnv Processing environment. + */ + void writeConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject fileObject; + try { + fileObject = filer.createResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + + try (OutputStream out = fileObject.openOutputStream()) { + BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(out, UTF_8)); + writeClassName(writer, messageGroupClassName); + writer.newLine(); + + for (ClassName messageClassName : messageClasses) { + writeClassName(writer, messageClassName); + writer.newLine(); + } + + writer.flush(); + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Reads configuration from disk. + * + * @param processingEnv Processing environment. + */ + static IncrementalCompilationConfig readConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject resource; + + try { + resource = filer.getResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + return null; + } + + try (Reader reader = resource.openReader(true)) { + BufferedReader bufferedReader = new BufferedReader(reader); + String messageClassNameString = bufferedReader.readLine(); + + if (messageClassNameString == null) { + return null; + } + + ClassName messageClassName = readClassName(messageClassNameString); + + List<ClassName> message = new ArrayList<>(); + + String line; + while ((line = bufferedReader.readLine()) != null) { + ClassName className = readClassName(line); + message.add(className); + } + + return new IncrementalCompilationConfig(messageClassName, message); + } catch (FileNotFoundException | NoSuchFileException e) { Review comment: Where did you find that? Nothing in this method's contract states that it throws these particular exceptions ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/IncrementalCompilationConfig.java ########## @@ -0,0 +1,176 @@ +/* + * 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.network.processor; + +import static java.util.stream.Collectors.toList; + +import com.squareup.javapoet.ClassName; +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.nio.file.NoSuchFileException; +import java.util.Arrays; +import java.util.List; +import javax.annotation.processing.Filer; +import javax.annotation.processing.ProcessingEnvironment; +import javax.tools.FileObject; +import javax.tools.StandardLocation; +import org.jetbrains.annotations.Nullable; + +/** + * Incremental configuration of the {@link TransferableObjectProcessor}. + * Holds data between (re-)compilations. + * <br> + * The serialized format of this config is as follows: + * <br> + * First line: message group class' name + * <br> + * Next lines: message class' names + * <br> + * Every class name is written as {@code packageName + " " + simpleName1 + " " + ... + simpleNameN}, e.g. + * "org.apache.ignite OuterClass InnerClass EvenMoreInnerClass". + */ +class IncrementalCompilationConfig { + /** Incremental compilation configuration file name. */ + static final String CONFIG_FILE_NAME = "META-INF/transferable.messages"; + + /** Message group class name. */ + private ClassName messageGroupClassName; + + /** Messages. */ + private final List<ClassName> messageClasses; + + IncrementalCompilationConfig(ClassName messageGroupClassName, List<ClassName> messageClasses) { + this.messageGroupClassName = messageGroupClassName; + this.messageClasses = List.copyOf(messageClasses); + } + + /** + * Saves configuration on disk. + * + * @param processingEnv Processing environment. + */ + void writeConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject fileObject; + try { + fileObject = filer.createResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + + try (BufferedWriter writer = new BufferedWriter(fileObject.openWriter())) { + writeClassName(writer, messageGroupClassName); + + for (ClassName messageClassName : messageClasses) { + writeClassName(writer, messageClassName); + } + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Reads configuration from disk. + * + * @param processingEnv Processing environment. + */ + @Nullable + static IncrementalCompilationConfig readConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject resource; + + try { + resource = filer.getResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + return null; + } + + try (BufferedReader bufferedReader = new BufferedReader(resource.openReader(true))) { + String messageClassNameString = bufferedReader.readLine(); + + if (messageClassNameString == null) { + return null; + } + + ClassName messageClassName = readClassName(messageClassNameString); + + List<ClassName> messages = bufferedReader.lines().map(IncrementalCompilationConfig::readClassName).collect(toList()); + + return new IncrementalCompilationConfig(messageClassName, messages); + } catch (FileNotFoundException | NoSuchFileException e) { + return null; + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Writes class name with all the enclosing classes. + * + * @param writer Writer. + * @param className Class name. + * @throws IOException If failed. + */ + private static void writeClassName(BufferedWriter writer, ClassName className) throws IOException { + writer.write(className.packageName()); + writer.write(' '); + + List<String> simpleNames = className.simpleNames(); Review comment: you can inline this variable ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/MessageClass.java ########## @@ -141,13 +141,22 @@ public String simpleName() { return getters; } + /** + * Returns class name that the generated SerializationFactory should have. + * + * @return Class name that the generated SerializationFactory should have. + */ + public ClassName serializationFactoryName() { + return ClassName.get(className.packageName(), className.simpleName() + "SerializationFactory"); + } + /** * Returns class name that the generated Network Message implementation should have. * * @return Class name that the generated Network Message implementation should have. */ public ClassName implClassName() { - return ClassName.get(packageName(), simpleName() + "Impl"); + return ClassName.get(className.packageName(), className.simpleName() + "Impl"); Review comment: I don't understand why these changes are needed =) ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/TransferableObjectProcessor.java ########## @@ -71,18 +76,26 @@ public SourceVersion getSupportedSourceVersion() { @Override public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) { try { + IncrementalCompilationConfig currentConfig = IncrementalCompilationConfig.readConfig(processingEnv); Review comment: Maybe it makes sense to move this call further down, for example, if the message list is empty ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/TransferableObjectProcessor.java ########## @@ -96,6 +109,39 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment return true; } + private void updateConfig(List<MessageClass> messages, MessageGroupWrapper messageGroup) { + List<ClassName> messageClassNames = messages.stream() + .map(MessageClass::className) + .collect(toList()); + + var config = new IncrementalCompilationConfig(ClassName.get(messageGroup.element()), messageClassNames); Review comment: see, you already treating this class as immutable here, why do you have to overwrite the message group class in the `getMessageGroup` method? ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/TransferableObjectProcessor.java ########## @@ -96,6 +109,39 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment return true; } + private void updateConfig(List<MessageClass> messages, MessageGroupWrapper messageGroup) { + List<ClassName> messageClassNames = messages.stream() + .map(MessageClass::className) + .collect(toList()); + + var config = new IncrementalCompilationConfig(ClassName.get(messageGroup.element()), messageClassNames); + + config.writeConfig(processingEnv); + } + + private List<MessageClass> mergeMessages(IncrementalCompilationConfig currentConfig, List<MessageClass> messages) { + List<ClassName> messageClassesFromConfig = new ArrayList<>(currentConfig.messageClasses()); Review comment: what's the point of copying the array here? ########## File path: modules/network/src/integrationTest/java/org/apache/ignite/internal/network/processor/ItTransferableObjectProcessorIncrementalTest.java ########## @@ -0,0 +1,339 @@ +/* + * 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.network.processor; + +import static org.apache.ignite.internal.network.processor.InMemoryJavaFileManager.uriForFileObject; +import static org.apache.ignite.internal.network.processor.InMemoryJavaFileManager.uriForJavaFileObject; +import static org.apache.ignite.internal.network.processor.IncrementalCompilationConfig.CONFIG_FILE_NAME; +import static org.apache.ignite.internal.network.processor.IncrementalCompilationConfig.readClassName; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.testing.compile.JavaFileObjects; +import com.squareup.javapoet.ClassName; +import java.io.BufferedReader; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import javax.tools.DiagnosticCollector; +import javax.tools.JavaCompiler; +import javax.tools.JavaCompiler.CompilationTask; +import javax.tools.JavaFileObject; +import javax.tools.JavaFileObject.Kind; +import javax.tools.StandardJavaFileManager; +import javax.tools.StandardLocation; +import javax.tools.ToolProvider; +import org.intellij.lang.annotations.Language; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Integration tests for the {@link TransferableObjectProcessor} incremental compilation. + */ +public class ItTransferableObjectProcessorIncrementalTest { + /** + * Package name of the test sources. + */ + private static final String RESOURCE_PACKAGE_NAME = "org.apache.ignite.internal.network.processor"; + + /** File manager for incremental compilation. */ + private InMemoryJavaFileManager fileManager; + + /** Javac diagnostic collector. */ + private DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>(); Review comment: ```suggestion private final DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>(); ``` ########## File path: modules/network/src/integrationTest/java/org/apache/ignite/internal/network/processor/ItTransferableObjectProcessorIncrementalTest.java ########## @@ -0,0 +1,339 @@ +/* + * 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.network.processor; + +import static org.apache.ignite.internal.network.processor.InMemoryJavaFileManager.uriForFileObject; +import static org.apache.ignite.internal.network.processor.InMemoryJavaFileManager.uriForJavaFileObject; +import static org.apache.ignite.internal.network.processor.IncrementalCompilationConfig.CONFIG_FILE_NAME; +import static org.apache.ignite.internal.network.processor.IncrementalCompilationConfig.readClassName; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.testing.compile.JavaFileObjects; +import com.squareup.javapoet.ClassName; +import java.io.BufferedReader; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import javax.tools.DiagnosticCollector; +import javax.tools.JavaCompiler; +import javax.tools.JavaCompiler.CompilationTask; +import javax.tools.JavaFileObject; +import javax.tools.JavaFileObject.Kind; +import javax.tools.StandardJavaFileManager; +import javax.tools.StandardLocation; +import javax.tools.ToolProvider; +import org.intellij.lang.annotations.Language; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Integration tests for the {@link TransferableObjectProcessor} incremental compilation. + */ +public class ItTransferableObjectProcessorIncrementalTest { + /** + * Package name of the test sources. + */ + private static final String RESOURCE_PACKAGE_NAME = "org.apache.ignite.internal.network.processor"; + + /** File manager for incremental compilation. */ + private InMemoryJavaFileManager fileManager; + + /** Javac diagnostic collector. */ + private DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>(); + + @BeforeEach + void setUp() { + JavaCompiler systemJavaCompiler = ToolProvider.getSystemJavaCompiler(); + StandardJavaFileManager standardFileManager = systemJavaCompiler.getStandardFileManager(diagnosticCollector, Locale.getDefault(), + StandardCharsets.UTF_8); + + this.fileManager = new InMemoryJavaFileManager(standardFileManager); + } + + @Test + public void testIncrementalRemoveTransferable() throws Exception { + String testMessageGroup = "MsgGroup"; + String testMessageGroupName = "GroupName"; + String testMessageClass = "TestMessage"; + String testMessageClass2 = "SomeMessage"; + + var compilationObjects1 = new ArrayList<JavaFileObject>(); + JavaFileObject messageGroupObject = createMessageGroup(testMessageGroup, testMessageGroupName); + compilationObjects1.add(messageGroupObject); + compilationObjects1.add(createTransferable(testMessageClass, 0)); + + Map<URI, JavaFileObject> compilation1 = compile(compilationObjects1); + + JavaFileObject messageRegistry1 = compilation1.get(uriForMessagesFile()); + try (BufferedReader bufferedReader = new BufferedReader(messageRegistry1.openReader(true))) { Review comment: I can't fully agree with this testing approach, since you are relying on the internal deserialization mechanism. I think that using mocks and `readConfig` might be a better solution, for example: ``` var env = mock(ProcessingEnvironment.class); var filer = mock(Filer.class); when(env.getFiler()).thenReturn(filer); when(filer.getResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME)) .thenReturn(messageRegistry1); var config = IncrementalCompilationConfig.readConfig(env); ``` What do you think? This way you won't have to use internal `readClassName` methods ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/IncrementalCompilationConfig.java ########## @@ -0,0 +1,176 @@ +/* + * 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.network.processor; + +import static java.util.stream.Collectors.toList; + +import com.squareup.javapoet.ClassName; +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.nio.file.NoSuchFileException; +import java.util.Arrays; +import java.util.List; +import javax.annotation.processing.Filer; +import javax.annotation.processing.ProcessingEnvironment; +import javax.tools.FileObject; +import javax.tools.StandardLocation; +import org.jetbrains.annotations.Nullable; + +/** + * Incremental configuration of the {@link TransferableObjectProcessor}. + * Holds data between (re-)compilations. + * <br> + * The serialized format of this config is as follows: + * <br> + * First line: message group class' name + * <br> + * Next lines: message class' names + * <br> + * Every class name is written as {@code packageName + " " + simpleName1 + " " + ... + simpleNameN}, e.g. + * "org.apache.ignite OuterClass InnerClass EvenMoreInnerClass". + */ +class IncrementalCompilationConfig { + /** Incremental compilation configuration file name. */ + static final String CONFIG_FILE_NAME = "META-INF/transferable.messages"; + + /** Message group class name. */ + private ClassName messageGroupClassName; + + /** Messages. */ + private final List<ClassName> messageClasses; + + IncrementalCompilationConfig(ClassName messageGroupClassName, List<ClassName> messageClasses) { + this.messageGroupClassName = messageGroupClassName; + this.messageClasses = List.copyOf(messageClasses); + } + + /** + * Saves configuration on disk. + * + * @param processingEnv Processing environment. + */ + void writeConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject fileObject; + try { + fileObject = filer.createResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + + try (BufferedWriter writer = new BufferedWriter(fileObject.openWriter())) { + writeClassName(writer, messageGroupClassName); + + for (ClassName messageClassName : messageClasses) { + writeClassName(writer, messageClassName); + } + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Reads configuration from disk. + * + * @param processingEnv Processing environment. + */ + @Nullable + static IncrementalCompilationConfig readConfig(ProcessingEnvironment processingEnv) { + Filer filer = processingEnv.getFiler(); + + FileObject resource; + + try { + resource = filer.getResource(StandardLocation.CLASS_OUTPUT, "", CONFIG_FILE_NAME); + } catch (IOException e) { + return null; + } + + try (BufferedReader bufferedReader = new BufferedReader(resource.openReader(true))) { + String messageClassNameString = bufferedReader.readLine(); + + if (messageClassNameString == null) { + return null; + } + + ClassName messageClassName = readClassName(messageClassNameString); + + List<ClassName> messages = bufferedReader.lines().map(IncrementalCompilationConfig::readClassName).collect(toList()); + + return new IncrementalCompilationConfig(messageClassName, messages); + } catch (FileNotFoundException | NoSuchFileException e) { + return null; + } catch (IOException e) { + throw new ProcessingException(e.getMessage()); + } + } + + /** + * Writes class name with all the enclosing classes. + * + * @param writer Writer. + * @param className Class name. + * @throws IOException If failed. + */ + private static void writeClassName(BufferedWriter writer, ClassName className) throws IOException { + writer.write(className.packageName()); + writer.write(' '); + + List<String> simpleNames = className.simpleNames(); + + for (String enclosingSimpleName : simpleNames) { + writer.write(enclosingSimpleName); + writer.write(' '); + } + + writer.newLine(); + } + + /** + * Reads class name. + * + * @param line Line. + * @return Class name. + */ + static ClassName readClassName(String line) { + String[] split = line.split(" "); + + String packageName = split[0]; + + String firstSimpleName = split[1]; + + String[] simpleNames = split.length > 2 ? Arrays.copyOfRange(split, 2, split.length) : new String[0]; + + return ClassName.get(packageName, firstSimpleName, simpleNames); + } + + ClassName messageGroupClassName() { + return messageGroupClassName; + } + + void messageGroupClassName(ClassName messageGroupClassName) { Review comment: Why don't you want to make this class immutable? I think that the current approach is a little bit overly complex - for example, you need to modify this config inside the `getMessageGroup` method ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/TransferableObjectProcessor.java ########## @@ -96,6 +109,39 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment return true; } + private void updateConfig(List<MessageClass> messages, MessageGroupWrapper messageGroup) { + List<ClassName> messageClassNames = messages.stream() + .map(MessageClass::className) + .collect(toList()); + + var config = new IncrementalCompilationConfig(ClassName.get(messageGroup.element()), messageClassNames); + + config.writeConfig(processingEnv); + } + + private List<MessageClass> mergeMessages(IncrementalCompilationConfig currentConfig, List<MessageClass> messages) { Review comment: Even though your approach is fine, it is possible to write this method shorter: ``` Elements elementUtils = processingEnv.getElementUtils(); Stream<MessageClass> configMessages = currentConfig.messageClasses().stream() .map(ClassName::canonicalName) .map(elementUtils::getTypeElement) .filter(element -> element != null && element.getAnnotation(Transferable.class) != null) .map(element -> new MessageClass(processingEnv, element)); return Stream.concat(messages.stream(), configMessages).collect(toSet()); ``` What do you think? ########## File path: modules/network/src/integrationTest/java/org/apache/ignite/internal/network/processor/ItTransferableObjectProcessorIncrementalTest.java ########## @@ -294,7 +294,7 @@ private JavaFileObject createNonTransferable(String className) { JavaCompiler systemJavaCompiler = ToolProvider.getSystemJavaCompiler(); CompilationTask task = systemJavaCompiler - .getTask(null, fileManager, diagnosticCollector, Collections.emptyList(), Set.of(), files); Review comment: please leave explicit `Set.of`, `of` is very hard to read -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
