DUBBO-7349 Fix the build on Linux ARM64 CPU architecture #7408

martin-g · 2021-03-18T11:57:07Z

What is the purpose of the change

Apache Dubbo is a Java application and thus should work fine on any architecture supported by the Java Virtual Machine.
But Dubbo depends on some libraries which use native binaries that do not have aarch64 version, for example embedded-redis and embedded-consul.
This PR updates those dependencies with newer versions which added support for ARM64 architecture!

Verifying this change

The PR re-introduces TravisCI as a build tool to build and tests master branch every night.
This has been discussed at https://lists.apache.org/thread.html/r2742c51066b55787571417ed74843cd9607ba8202566ddd807f9bdab%40%3Cdev.dubbo.apache.org%3E
Someone from Dubbo team will have to setup the Cron job at TravisCI UI !

AlbumenJ · 2021-03-18T12:37:53Z

dubbo-metadata/dubbo-metadata-report-redis/pom.xml

            <artifactId>embedded-redis</artifactId>
-            <scope>test</scope>


why remove test scope

because it is already set at https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-dependencies-bom/pom.xml#L628 in the dependencyManagement section

this way it is in sync with

https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-registry/dubbo-registry-multiple/pom.xml#L55-L58

https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-registry/dubbo-registry-redis/pom.xml#L47-L50

https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-rpc/dubbo-rpc-redis/pom.xml#L42-L45

Rather than remove <scope>test</scope>, add <scope>test</scope> will be better I think.

https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-registry/dubbo-registry-multiple/pom.xml#L55-L58

https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-registry/dubbo-registry-redis/pom.xml#L47-L50

https://github.com/martin-g/dubbo/blob/fix-build-and-tests-on-arm64/dubbo-rpc/dubbo-rpc-redis/pom.xml#L42-L45

@horizonzy OK, I will add the scope!

Done!
Also rebased to latest master

dubbo-dependencies-bom/pom.xml

…gement section Inline the version so that it is not forgotten to be removed when the dependency is removed with the upgrade to embedded-consul:2.2.1 See pszymczyk/embedded-consul#114

szpak · 2021-03-18T17:37:44Z

dubbo-dependencies-bom/pom.xml

@@ -700,6 +700,16 @@
                <artifactId>grpc-grpclb</artifactId>
                <version>${grpc.version}</version>
            </dependency>
+            <!-- Groovy, for Embedded Consul. Could be removed once embedded-consul:2.2.1 is released
+             See https://github.com/pszymczyk/embedded-consul/pull/114


You might wait a little bit with merging that PR. We plan to release 2.2.1 "soon".

I really hope this PR can be merged ASAP, so this issue #7421 can be fixed.

@pinxiong I am afraid that this PR won't fix the issue for Mac ARM64. Currently embedded-redis:0.9.0 supports only Linux ARM64. We used Docker+QEMU to build the Linux ARM64 binary - https://github.com/codemonstur/embedded-redis/tree/master/src/main/binaries. I am not sure whether there is a similar way to emulate Mac ARM64.
I am not sure whether @codemonstur has access to Mac M1 machine to build the binary.

Thank you for letting me know! If somebody need my help for testing in Mac M1 machine, I'm very happy to do it. @martin-g @codemonstur

@pinxiong With PR codemonstur/embedded-redis#3 I've tried to convince @codemonstur to add support for a special system property to ExecutableProvider to set the path to the redis-server binary but he didn't like this approach.
If this PR was merged then Dubbo/pom.xml could add a profile for Mac ARM64 and export this property in Maven Surefire argline. Maybe @codemonstur will reconsider it.

BTW once you pass the issue with embedded-redis I expect that you will face similar issue with embedded-consul.

BTW once you pass the issue with embedded-redis I expect that you will face similar issue with embedded-consul.

@martin-g Thanks for your reminder.

@martin-g I have passed the issue with embedded-redis when I disabled all test cases in RedisMetadataReportTest. Fortunately, I didn't face similar issue with embedded-consul.

You might wait a little bit with merging that PR. We plan to release 2.2.1 "soon".

@szpak Any ETA ?

@martin-g ETA: -15 minutes :-)

https://github.com/pszymczyk/embedded-consul/releases/tag/release%2F2.2.1

Thank you, @szpak!
I've upgraded to 2.2.1 with 8825d52

@AlbumenJ The PR is ready for review and once the CI finishes green for merging!

codemonstur · 2021-03-22T14:29:49Z

...eport-redis/src/test/java/org/apache/dubbo/metadata/store/redis/RedisMetadataReportTest.java

@@ -60,15 +60,15 @@ public void constructor(TestInfo testInfo) throws IOException {
        String methodName = testInfo.getTestMethod().get().getName();


Below this line you would put

ExecutableProvider provider = () -> new File(System.getProperty("my.custom.property"));

codemonstur · 2021-03-22T14:30:24Z

...eport-redis/src/test/java/org/apache/dubbo/metadata/store/redis/RedisMetadataReportTest.java

@@ -60,15 +60,15 @@ public void constructor(TestInfo testInfo) throws IOException {
        String methodName = testInfo.getTestMethod().get().getName();
        if ("testAuthRedisMetadata".equals(methodName) || ("testWrongAuthRedisMetadata".equals(methodName))) {
            String password = "チェリー";
-            RedisServerBuilder builder = RedisServer.builder().port(redisPort).setting("requirepass " + password);
+            RedisServerBuilder builder = RedisServer.newRedisServer().port(redisPort).setting("requirepass " + password);


Add .redisExecProvider(provider) here.

codemonstur · 2021-03-22T14:30:43Z

...eport-redis/src/test/java/org/apache/dubbo/metadata/store/redis/RedisMetadataReportTest.java

            if (SystemUtils.IS_OS_WINDOWS) {
                // set maxheap to fix Windows error 0x70 while starting redis
                builder.setting("maxheap 128mb");
            }
            redisServer = builder.build();
            registryUrl = URL.valueOf("redis://username:" + password + "@localhost:" + redisPort);
        } else {
-            RedisServerBuilder builder = RedisServer.builder().port(redisPort);
+            RedisServerBuilder builder = RedisServer.newRedisServer().port(redisPort);


And here add .redisExecProvider(provider) as well.

codemonstur · 2021-03-22T14:31:55Z

...eport-redis/src/test/java/org/apache/dubbo/metadata/store/redis/RedisMetadataReportTest.java

@@ -60,15 +60,15 @@ public void constructor(TestInfo testInfo) throws IOException {
        String methodName = testInfo.getTestMethod().get().getName();
        if ("testAuthRedisMetadata".equals(methodName) || ("testWrongAuthRedisMetadata".equals(methodName))) {
            String password = "チェリー";
-            RedisServerBuilder builder = RedisServer.builder().port(redisPort).setting("requirepass " + password);
+            RedisServerBuilder builder = RedisServer.newRedisServer().port(redisPort).setting("requirepass " + password);
            if (SystemUtils.IS_OS_WINDOWS) {


Yuck. I'm going to add a .settingIf(boolean, String) so that you can use the builder as it was intended.

@codemonstur Thanks for your help. I hope everything will be the same as you expected.

@codemonstur I see that your update has been merged in master branch. However, I still cannot work well when I run RedisMetadataReportTest.

If we want to fix issue #7421 , we need to upgrade the redis from 2.8.19 to 6.0.10 or 6.2 RC2 (see #8062, #8195) in embedded-redis

We can get some more details from redis releases 6.0.10

Bug fixes:

Fix setproctitle related crashes. (#8150, #8088)
Caused various crashes on startup, mainly on Apple M1 chips or under instrumentation

I really hope you can commit an new PR to solve this problem (#7421).

Maybe @martin-g can persuade @codemonstur to do it.

I was going to suggest to create an issue at https://github.com/codemonstur/embedded-redis and discuss there but "Issues" are not enabled there, only "Pull Requests".

@pinxiong As I said earlier - many of us do not have access to Mac ARM64 hardware and it is impossible for us to improve for it. But @codemonstur added APIs to use custom ExecutableProvider that can run any binary you tell it to.
For example, you can introduce a new Maven profile that is activated only for os=mac + arch=aarch64 and sets system property embedded.redis.executable (see https://github.com/codemonstur/embedded-redis/blob/master/src/main/java/redis/embedded/core/ExecutableProvider.java#L18). The value should be an absolute path to the Redis 6.x binary for Mac ARM64. Then in the tests code check whether this system property is non-null and use ExecutableProvider.newSystemPropertyProvider() instead of the default one.

@pinxiong the current approach that I think will work for you is as @martin-g described. I just enabled issues in https://github.com/codemonstur/embedded-redis. So if you guys want to discuss any it is now possible.

@martin-g @codemonstur I have opened an issue #6 where we can discuss.

AlbumenJ · 2021-04-06T08:30:31Z

@martin-g great! Please merge master branch to solve confilcting file. README.md is confilcted with master branch.

…gement section Inline the version so that it is not forgotten to be removed when the dependency is removed with the upgrade to embedded-consul:2.2.1 See pszymczyk/embedded-consul#114

martin-g · 2021-04-06T08:39:45Z

Done!

codecov-io · 2021-04-06T09:17:45Z

Codecov Report

Merging #7408 (a70d2c6) into master (0f5040b) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7408      +/-   ##
============================================
- Coverage     59.24%   59.19%   -0.06%     
+ Complexity      503      501       -2     
============================================
  Files          1076     1076              
  Lines         43407    43406       -1     
  Branches       6337     6337              
============================================
- Hits          25716    25693      -23     
- Misses        14852    14877      +25     
+ Partials       2839     2836       -3

Impacted Files	Coverage Δ	Complexity Δ
.../dubbo/registry/consul/ConsulServiceDiscovery.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...che/dubbo/remoting/transport/mina/MinaChannel.java	`35.52% <0.00%> (-10.53%)`	`14.00% <0.00%> (-1.00%)`
...dubbo/remoting/exchange/support/DefaultFuture.java	`80.34% <0.00%> (-6.84%)`	`0.00% <0.00%> (ø%)`
...he/dubbo/remoting/transport/netty/NettyServer.java	`70.17% <0.00%> (-3.51%)`	`8.00% <0.00%> (-1.00%)`
...e/dubbo/registry/integration/RegistryProtocol.java	`58.35% <0.00%> (-2.84%)`	`0.00% <0.00%> (ø%)`
...e/dubbo/remoting/transport/netty4/NettyClient.java	`69.56% <0.00%> (-1.45%)`	`0.00% <0.00%> (ø%)`
...pache/dubbo/registry/support/FailbackRegistry.java	`65.42% <0.00%> (-0.94%)`	`0.00% <0.00%> (ø%)`
...apache/dubbo/common/extension/ExtensionLoader.java	`77.95% <0.00%> (-0.45%)`	`0.00% <0.00%> (ø%)`
...n/java/org/apache/dubbo/common/utils/UrlUtils.java	`74.14% <0.00%> (-0.35%)`	`0.00% <0.00%> (ø%)`
...ubbo/registry/client/ServiceDiscoveryRegistry.java	`42.85% <0.00%> (+0.59%)`	`0.00% <0.00%> (ø%)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f5040b...a70d2c6. Read the comment docs.

AlbumenJ

LGTM.

Update embedded-redis and embedded-consul to newer versions which have native binaries for aarch64 Re-introduce TravisCI as a build tool only for ARM64. It will run as a cron job every night

…gement section Inline the version so that it is not forgotten to be removed when the dependency is removed with the upgrade to embedded-consul:2.2.1 See pszymczyk/embedded-consul#114

horizonzy

LGTM.

…ions And that there is a nightly build on ARM64 at TravisCI Depends-on: apache/dubbo#7408

AlbumenJ reviewed Mar 18, 2021

View reviewed changes

szpak reviewed Mar 18, 2021

View reviewed changes

kevinw66 mentioned this pull request Mar 21, 2021

Can't start redis server. Check logs for details. #7421

Closed

2 tasks

martin-g changed the title ~~DUBBO-7349 Fix the build on ARM64 CPU architecture~~ DUBBO-7349 Fix the build on Linux ARM64 CPU architecture Mar 22, 2021

codemonstur reviewed Mar 22, 2021

View reviewed changes

martin-g force-pushed the fix-build-and-tests-on-arm64 branch from 8825d52 to f959bf5 Compare April 6, 2021 08:38

AlbumenJ approved these changes Apr 10, 2021

View reviewed changes

martin-g added 5 commits April 12, 2021 08:43

Issue apache#7349 - Fix the build on ARM64 CPU architecture

20635e0

Update embedded-redis and embedded-consul to newer versions which have native binaries for aarch64 Re-introduce TravisCI as a build tool only for ARM64. It will run as a cron job every night

Issue apache#7408 - Set the scope of groovy-xml in the dependencyMana…

d0fab31

…gement section Inline the version so that it is not forgotten to be removed when the dependency is removed with the upgrade to embedded-consul:2.2.1 See pszymczyk/embedded-consul#114

Update embedded-redis to 0.10.0

f1678da

DUBBO-7349 Upgrade Embedded-Consul to 2.2.1

bcaf9b9

DUBBO-7349 Add scope=test to all occurrences of embedded-[redis|consul]

a70d2c6

martin-g force-pushed the fix-build-and-tests-on-arm64 branch from f959bf5 to a70d2c6 Compare April 12, 2021 05:45

horizonzy approved these changes Apr 12, 2021

View reviewed changes

martin-g added a commit to martin-g/dubbo-website that referenced this pull request Apr 14, 2021

DUBBO-7349 Add a note that all Pull Requests are tested at GitHub Act…

208ba4a

…ions And that there is a nightly build on ARM64 at TravisCI Depends-on: apache/dubbo#7408

martin-g mentioned this pull request Apr 14, 2021

DUBBO-7349 Add a note that all Pull Requests are tested at GitHub Actions apache/dubbo-website#775

Merged

AlbumenJ merged commit 062166f into apache:master Apr 15, 2021

CrazyHZM mentioned this pull request Sep 30, 2021

Dubbo framework support for ARM architecture discussion #7349

Closed

martin-g deleted the fix-build-and-tests-on-arm64 branch September 30, 2021 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DUBBO-7349 Fix the build on Linux ARM64 CPU architecture #7408

DUBBO-7349 Fix the build on Linux ARM64 CPU architecture #7408

martin-g commented Mar 18, 2021

AlbumenJ Mar 18, 2021

martin-g Mar 18, 2021

martin-g Mar 18, 2021

horizonzy Apr 11, 2021

martin-g Apr 12, 2021

martin-g Apr 12, 2021

szpak Mar 18, 2021 •

edited

Loading

pinxiong Mar 22, 2021

martin-g Mar 22, 2021

pinxiong Mar 22, 2021

martin-g Mar 22, 2021

pinxiong Mar 23, 2021

pinxiong Mar 23, 2021

martin-g Apr 1, 2021

szpak Apr 6, 2021

martin-g Apr 6, 2021

codemonstur Mar 22, 2021 •

edited

Loading

codemonstur Mar 22, 2021

codemonstur Mar 22, 2021

codemonstur Mar 22, 2021

pinxiong Mar 23, 2021

pinxiong Apr 16, 2021

martin-g Apr 16, 2021 •

edited

Loading

codemonstur Apr 16, 2021

pinxiong Apr 16, 2021

AlbumenJ commented Apr 6, 2021

martin-g commented Apr 6, 2021

codecov-io commented Apr 6, 2021 •

edited

Loading

AlbumenJ left a comment

horizonzy left a comment

		@@ -60,15 +60,15 @@ public void constructor(TestInfo testInfo) throws IOException {
		String methodName = testInfo.getTestMethod().get().getName();

DUBBO-7349 Fix the build on Linux ARM64 CPU architecture #7408

DUBBO-7349 Fix the build on Linux ARM64 CPU architecture #7408

Conversation

martin-g commented Mar 18, 2021

What is the purpose of the change

Verifying this change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemonstur Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-g Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbumenJ commented Apr 6, 2021

martin-g commented Apr 6, 2021

codecov-io commented Apr 6, 2021 • edited Loading

Codecov Report

AlbumenJ left a comment

Choose a reason for hiding this comment

horizonzy left a comment

Choose a reason for hiding this comment

szpak Mar 18, 2021 •

edited

Loading

codemonstur Mar 22, 2021 •

edited

Loading

martin-g Apr 16, 2021 •

edited

Loading

codecov-io commented Apr 6, 2021 •

edited

Loading