Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GraalVM Reachability Metadata and corresponding nativeTest for Seata integration #30138

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

linghengqian
Copy link
Member

@linghengqian linghengqian commented Feb 15, 2024

For #29052.

Changes proposed in this pull request:

[INFO] Results:
[INFO] 
[INFO] Tests run: 14, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[WARNING] Corrupted channel by directly writing to native stream in forked JVM 1. See FAQ web page and the dump file /home/linghengqian/TwinklingLiftWorks/git/public/shardingsphere/test/native/target/surefire-reports/2024-02-15T23-36-35_487-jvmRun1.dumpstream
[INFO] 
[INFO] --- native-maven-plugin:0.10.0:merge-agent-files (test-native) @ shardingsphere-test-native ---
[INFO] Merging agent 1 files into /home/linghengqian/TwinklingLiftWorks/git/public/shardingsphere/test/native/target/native/agent-output/test
[INFO] 
[INFO] --- native-maven-plugin:0.10.0:test (test-native) @ shardingsphere-test-native ---
[INFO] Skipping native-image tests (parameter 'skipTests' or 'skipNativeTests' is true).
[INFO] 
[INFO] --- native-maven-plugin:0.10.0:metadata-copy (default-cli) @ shardingsphere-test-native ---
[INFO] Found GraalVM installation from JAVA_HOME variable.
[INFO] Copying files from: test
[ERROR] Metadata copy process failed with code: 1

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.

@slievrly
Copy link
Member

For #29052.

Changes proposed in this pull request:

  • Add GraalVM Reachability Metadata and corresponding nativeTest for Seata integration.
  • Remove inappropriate reference to com.mysql.cj.jdbc.exceptions.CommunicationsException.
  • Uses io.seata:seata-all:1.7.1 and make corresponding adjustments in nativeTests. Before merging feature: First support native-image for seata-client incubator-seata#5234, using Seata Client under GraalVM Native Image requires using the version released under its native branch. Using io.seata:seata-all:2.0.0 under nativeTest needs to wait for the ByteBuddy Java API to be removed on the seata side.

Seata 1.x supports native image from version 1.7.0, refer to the merged PR apache/incubator-seata#5476 (comment). Currently, 2.x does not support native image, and this function of 1.x will be translated in the future. For native image integration, I recommend using 1.8.0, the latest version of 1.x.

  • seata's configuration file file.conf has no dynamically defined Java API, which prevents the integration of testcontainers.

The client-side uses the Seata SDK, which can depend on Seata in two ways:

  1. By using seata-all.jar, it will read registry-$env.conf (with the default value being registry.conf), which defines how Seata accesses the metadata from the registry and configuration centers. When config.type=xxx is read from the configuration center and xxx is set to 'file', it will by default read the configuration from file.conf located in the same directory (though other paths and filenames can be specified). This does not support dynamic refresh of the configuration. When xxx is set to a third-party configuration center, some configurations support dynamic refresh, but most configurations are read-once types during startup. Currently, it does not support using Java API to modify configurations and reload them directly.

  2. By using seata-spring-boot-starter.jar. This defaults to including seata-all.jar dependency and adds autoconfiguration features for Spring Boot. It supports directly using Spring Boot's configuration files (either properties or yml) for configuration. Refer to https://github.com/apache/incubator-seata/tree/2.x/script/client/spring for examples. You can consider storing these configurations in your application's configuration center, where some configurations support dynamic refresh. Likewise, it does not support using Java API to modify configurations and reload them directly.

Furthermore, the priority of Seata client configuration is ENV (System.getenv()) > -D (System.getProperty()) > file or configuration center. You can consider injecting environment variables in testContainer.

@funky-eyes
Copy link

Is this the problem you're having? raphw/byte-buddy#1588

@linghengqian
Copy link
Member Author

Is this the problem you're having? raphw/byte-buddy#1588

Copy link
Member Author

@linghengqian linghengqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Even if the free local port is dynamically found through Java API code, file.conf still needs to be dynamically created after passing it to the Java API of testcontainers-java, which seems to break some of the conventions of GraalVM Native Image to a certain extent.
  • I'm worried about encountering insufficient Unix permissions like io.etcd.jetcd.launcher.EtcdContainer -- Error deleting directory when using jetcd-tests on unit-testing. etcd-io/jetcd#1310 . testcontainers-java always expects to use operating system-specific FileSystem like sun.nio.fs.UnixFileSystem/file:/resources/{entry}, but in fact we can only use com.oracle.svm.core.jdk.resources.NativeImageResourceFileSystem/resource:file:/resources/{entry} in NativeTest .
  • I don't think this is where application.yml should be introduced, although we can use a separate Spring Boot bootstrap class in a separate class.
  • This is mainly because this requires the introduction of org.springframework.boot:spring-boot-maven-plugin to use the Maven Goal of process-test-aot, which I think complicates things to a certain extent, because it seems best to have a module like shardingsphere-test-native-spring-boot-starter to additionally define a Maven Profile for writing nativeTest.
  • As a temporary measure, this unit test uses the hard-defined port 39567 and is restricted to running under Native Image only because it involves the creation of the Docker Image testcontainers/ryuk. Let me explore later what APIs are available on the Seata side that can be extended.

@linghengqian linghengqian marked this pull request as ready for review February 21, 2024 16:38
@tristaZero tristaZero merged commit ab0ed3d into apache:master Feb 22, 2024
140 checks passed
@linghengqian linghengqian deleted the seata branch February 22, 2024 02:32
@linghengqian linghengqian added this to the 5.5.0 milestone Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants