[AMORO-2008]: Refactor the optimizer module to separate distributing pacakges for different engines. #2051

baiyangtx · 2023-09-28T07:13:47Z

Why are the changes needed?

Close #2008 .

Brief change log

The optimizer module has been reorganized and split into ams/optimizer-container, ams/optimizer-job/job-common, ams/optimizer-job/flink-job, and ams/optimizer-job/standalone-job.
Changes have been made to the configuration files such as config.yaml, helm configmap, etc., to update the package name of localOptimizerContainer.
The dist directory has been modified, and the plugin/optimize/OptimizeJob.jar has been removed and replaced with plugin/optimize/flink/optimize-job.jar, which is only used for the Flink optimizer.

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request

Documentation

Does this pull request introduce a new feature? (no)

This reverts commit bbaa7f9.

codecov · 2023-09-28T07:55:43Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
...optimizer/container/AbstractResourceContainer.java	`2.17% <ø> (ø)`
...m/netease/arctic/optimizer/container/ExecUtil.java	`0.00% <ø> (ø)`
.../netease/arctic/optimizer/container/FlinkConf.java	`86.20% <ø> (ø)`
...c/optimizer/container/LocalOptimizerContainer.java	`0.00% <ø> (ø)`
...tease/arctic/optimizer/container/PropertyUtil.java	`0.00% <ø> (ø)`
...ic/optimizer/common/AbstractOptimizerOperator.java	`58.73% <ø> (ø)`
...com/netease/arctic/optimizer/common/Optimizer.java	`91.66% <ø> (ø)`
...tease/arctic/optimizer/common/OptimizerConfig.java	`72.34% <ø> (ø)`
...ase/arctic/optimizer/common/OptimizerExecutor.java	`83.33% <ø> (ø)`
...ease/arctic/optimizer/common/OptimizerToucher.java	`80.39% <ø> (ø)`
... and 1 more

... and 4 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

shidayang · 2023-09-28T09:14:09Z

ams/optimizer-container/pom.xml

+    <properties>
+        <fastjson.version>1.2.58</fastjson.version>
+        <avro.version>1.10.1</avro.version>
+        <snakeyaml.version>1.30</snakeyaml.version>


According to the current specification, all dependencies must be added to the dependencyManagement.

Yes, #2030 has move these properties to parent pom.

zhoujinsong · 2023-10-07T02:28:55Z

@baiyangtx Thanks a lot for your work!

Optimizer job seems to be a new concept for Amoro developers and users. Here, I think using Optimizer directly seems good enough. So I suggested to name the module to :

ams/optimizer-container
ams/optimizer/common
ams/optimizer/standalone-optimizer
ams/optimizer/flink-optimizer

How do you think?

baiyangtx · 2023-10-09T02:51:10Z

@baiyangtx Thanks a lot for your work!

Optimizer job seems to be a new concept for Amoro developers and users. Here, I think using Optimizer directly seems good enough. So I suggested to name the module to :
* ams/optimizer-container

* ams/optimizer/common

* ams/optimizer/standalone-optimizer

* ams/optimizer/flink-optimizer
How do you think?

I agree with it. I will fix it.

# Conflicts: # dist/src/main/arctic-bin/bin/optimizer.sh

…optimizer-module-refactor

zhoujinsong

@baiyangtx I left some comments.
BTW, you may need to modify the managing-optimizer.md docs too.

zhoujinsong · 2023-10-09T08:44:27Z

ams/optimizer-container/pom.xml

+    <properties>
+        <fastjson.version>1.2.58</fastjson.version>
+        <avro.version>1.10.1</avro.version>
+        <snakeyaml.version>1.30</snakeyaml.version>


Yes, #2030 has move these properties to parent pom.

zhoujinsong · 2023-10-09T08:46:48Z

ams/optimizer/common/src/main/java/com/netease/arctic/optimizer/job/common/Optimizer.java

@@ -1,4 +1,4 @@
-package com.netease.arctic.optimizer;
+package com.netease.arctic.optimizer.job.common;


Package com.netease.arctic.optimizer.job.common seems to be too long for me, com.netease.arctic.optimizer is good enough for me.

zhoujinsong · 2023-10-09T08:47:37Z

ams/optimizer/flink-optimizer/pom.xml

-    <artifactId>amoro-ams-optimizer</artifactId>
-    <name>Amoro Project Optimizer</name>
+    <artifactId>flink-optimizer</artifactId>
+    <name>Amoro Project Flink Optimizer Job</name>


Suggested change

<name>Amoro Project Flink Optimizer Job</name>

<name>Amoro Project Flink Optimizer</name>

zhoujinsong · 2023-10-09T08:48:13Z

ams/optimizer/standalone-optimizer/pom.xml

+    <modelVersion>4.0.0</modelVersion>
+
+    <artifactId>standalone-optimizer</artifactId>
+    <name>Amoro Project Standalone Optimizer Job</name>


Suggested change

<name>Amoro Project Standalone Optimizer Job</name>

<name>Amoro Project Standalone Optimizer</name>

…optimizer-module-refactor

zhoujinsong · 2023-10-09T09:47:17Z

ams/optimizer/common/pom.xml

+    <modelVersion>4.0.0</modelVersion>
+
+    <artifactId>optimizer-common</artifactId>
+    <name>Amoro Project Optimize Job Common</name>


Suggested change

<name>Amoro Project Optimize Job Common</name>

<name>Amoro Project Optimizer Common</name>

ams/optimizer-container/pom.xml

zhoujinsong · 2023-10-09T09:48:33Z

ams/optimizer-container/pom.xml

+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <scope>test</scope>


scope can be ommited.

zhoujinsong · 2023-10-09T09:49:10Z

ams/optimizer/common/pom.xml

+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <scope>test</scope>


Scope can be omited.

zhoujinsong · 2023-10-09T09:54:59Z

ams/optimizer/common/pom.xml

+        </dependency>
+    </dependencies>
+
+    <build>


Do we need to declare this plugin here and all other modules?

zhoujinsong

LGTM.

…pacakges for different engines. (apache#2051) * refactor modules * module name refactor * adjust pom.xml * add checkstyle configs * Revert "add checkstyle configs" This reverts commit bbaa7f9. * checkstyle problems * fix package * local -> standalone optimizer * local-optimizer-container name * refactor code struct as of review comment. * fix unit tests * fix package script * fix review comments * fix ut error * fix ut * fix rename * remove useless import * fix review comments * fix review comments

refactor modules

d55f6d1

github-actions bot added type:build module:ams-dashboard Ams dashboard module labels Sep 28, 2023

baiyangtx added 3 commits September 28, 2023 15:20

module name refactor

69e45f3

adjust pom.xml

7b62477

add checkstyle configs

bbaa7f9

github-actions bot added the module:ams-server Ams server module label Sep 28, 2023

baiyangtx added 2 commits September 28, 2023 15:43

Revert "add checkstyle configs"

eae9c0b

This reverts commit bbaa7f9.

checkstyle problems

29eee02

github-actions bot removed the module:ams-server Ams server module label Sep 28, 2023

fix package

4195415

baiyangtx added 2 commits September 28, 2023 16:03

local -> standalone optimizer

238e3af

local-optimizer-container name

df3da09

baiyangtx marked this pull request as ready for review September 28, 2023 09:03

shidayang reviewed Sep 28, 2023

View reviewed changes

baiyangtx added 7 commits October 9, 2023 10:56

refactor code struct as of review comment.

c487224

Merge branch 'master' into optimizer-module-refactor

a1b6f96

Merge branch 'master' into optimizer-module-refactor

4ef8cfe

# Conflicts: # dist/src/main/arctic-bin/bin/optimizer.sh

fix unit tests

aea9f3b

Merge branch 'master' into optimizer-module-refactor

c23ba29

fix package script

153cae2

Merge remote-tracking branch 'origin/optimizer-module-refactor' into …

05c2591

…optimizer-module-refactor

zhoujinsong reviewed Oct 9, 2023

View reviewed changes

baiyangtx added 3 commits October 9, 2023 17:09

Merge branch 'master' into optimizer-module-refactor

a731be4

fix review comments

a1120ad

Merge remote-tracking branch 'origin/optimizer-module-refactor' into …

b9d88a2

…optimizer-module-refactor

github-actions bot added the type:docs Improvements or additions to documentation label Oct 9, 2023

baiyangtx added 4 commits October 9, 2023 17:27

fix ut error

0e8503c

fix ut

bb7c038

fix rename

79c3667

remove useless import

283a4c7

zhoujinsong reviewed Oct 9, 2023

View reviewed changes

baiyangtx added 2 commits October 9, 2023 18:46

fix review comments

8f2deaf

fix review comments

e8a8307

zhoujinsong approved these changes Oct 9, 2023

View reviewed changes

baiyangtx merged commit 835ff61 into apache:master Oct 9, 2023

baiyangtx deleted the optimizer-module-refactor branch October 9, 2023 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMORO-2008]: Refactor the optimizer module to separate distributing pacakges for different engines. #2051

[AMORO-2008]: Refactor the optimizer module to separate distributing pacakges for different engines. #2051

baiyangtx commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

shidayang Sep 28, 2023

zhoujinsong Oct 9, 2023

zhoujinsong commented Oct 7, 2023 •

edited

Loading

baiyangtx commented Oct 9, 2023

zhoujinsong left a comment

zhoujinsong Oct 9, 2023

zhoujinsong Oct 9, 2023 •

edited

Loading

zhoujinsong Oct 9, 2023

baiyangtx Oct 9, 2023

zhoujinsong Oct 9, 2023

baiyangtx Oct 9, 2023

zhoujinsong Oct 9, 2023

baiyangtx Oct 9, 2023

zhoujinsong Oct 9, 2023

baiyangtx Oct 9, 2023

zhoujinsong Oct 9, 2023

baiyangtx Oct 9, 2023

zhoujinsong Oct 9, 2023

baiyangtx Oct 9, 2023

zhoujinsong left a comment

		@@ -1,4 +1,4 @@
		package com.netease.arctic.optimizer;
		package com.netease.arctic.optimizer.job.common;

	<name>Amoro Project Flink Optimizer Job</name>
	<name>Amoro Project Flink Optimizer</name>

	<name>Amoro Project Standalone Optimizer Job</name>
	<name>Amoro Project Standalone Optimizer</name>

	<name>Amoro Project Optimize Job Common</name>
	<name>Amoro Project Optimizer Common</name>

[AMORO-2008]: Refactor the optimizer module to separate distributing pacakges for different engines. #2051

[AMORO-2008]: Refactor the optimizer module to separate distributing pacakges for different engines. #2051

Conversation

baiyangtx commented Sep 28, 2023 • edited Loading

Why are the changes needed?

Brief change log

How was this patch tested?

Documentation

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhoujinsong commented Oct 7, 2023 • edited Loading

baiyangtx commented Oct 9, 2023

zhoujinsong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhoujinsong Oct 9, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhoujinsong left a comment

Choose a reason for hiding this comment

baiyangtx commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

zhoujinsong commented Oct 7, 2023 •

edited

Loading

zhoujinsong Oct 9, 2023 •

edited

Loading