[Refactor] Make coordinator class more organized #386

smallzhongfeng · 2022-12-04T11:19:21Z

What changes were proposed in this pull request?

Split coordinator class into different folders by their correlation.

Why are the changes needed?

To resolve #384

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Origin tests.

codecov-commenter · 2022-12-04T11:43:12Z

Codecov Report

Merging #386 (3312b73) into master (9a552b4) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #386      +/-   ##
============================================
- Coverage     58.63%   58.62%   -0.02%     
  Complexity     1590     1590              
============================================
  Files           193      193              
  Lines         10920    10921       +1     
  Branches        958      958              
============================================
- Hits           6403     6402       -1     
- Misses         4138     4139       +1     
- Partials        379      380       +1

Impacted Files	Coverage Δ
.../org/apache/uniffle/coordinator/AccessManager.java	`94.28% <ø> (ø)`
.../apache/uniffle/coordinator/CoordinatorServer.java	`65.26% <ø> (ø)`
...ava/org/apache/uniffle/coordinator/ServerNode.java	`83.78% <ø> (ø)`
...ache/uniffle/coordinator/SimpleClusterManager.java	`86.82% <ø> (ø)`
.../uniffle/coordinator/access/AccessCheckResult.java	`92.30% <ø> (ø)`
.../apache/uniffle/coordinator/access/AccessInfo.java	`92.30% <ø> (ø)`
...rdinator/access/checker/AbstractAccessChecker.java	`100.00% <ø> (ø)`
...inator/access/checker/AccessCandidatesChecker.java	`85.50% <ø> (ø)`
...nator/access/checker/AccessClusterLoadChecker.java	`96.96% <ø> (ø)`
...coordinator/access/checker/AccessQuotaChecker.java	`94.73% <ø> (ø)`
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

smallzhongfeng · 2022-12-04T12:53:04Z

Error:  High: org.apache.uniffle.coordinator.ApplicationManager.REMOTE_PATH_SCHEMA is a mutable collection [org.apache.uniffle.coordinator.ApplicationManager] At ApplicationManager.java:[line 53] MS_MUTABLE_COLLECTION

Waht could I resolve this? I haven't met before. @zuston

zuston · 2022-12-05T06:51:59Z

Error:  High: org.apache.uniffle.coordinator.ApplicationManager.REMOTE_PATH_SCHEMA is a mutable collection [org.apache.uniffle.coordinator.ApplicationManager] At ApplicationManager.java:[line 53] MS_MUTABLE_COLLECTION

Waht could I resolve this? I haven't met before. @zuston

Make it final?

zuston · 2022-12-05T07:00:33Z

Should we put checker and access folders together? Because they are related.
Another concern is the package renaming, which will make this PR incompatible with original config, like ‘org.apache.uniffle.coordinator.checker.AccessClusterLoadChecker’. Further more, I suggest that we'd better not to use the package name as the config of strategy in the future @jerqi Do you think so?

jerqi · 2022-12-05T07:10:29Z

Should we put checker and access folders together? Because they are related.

Another concern is the package renaming, which will make this PR incompatible with original config, like ‘org.apache.uniffle.coordinator.checker.AccessClusterLoadChecker’. Further more, I suggest that we'd better not to use the package name as the config of strategy in the future @jerqi Do you think so?

move checker directory to access directory?
Em..it's ok for me actually. We can add a document to explain how migrate to higher version if we make some imcompatible changes. Or we add short name for some configs. Or we introduce the deprecated configuration option.

zuston · 2022-12-05T07:43:26Z

+1. Short name or concrete strategy name will be better.

jerqi · 2022-12-05T07:50:55Z

+1. Short name or concrete strategy name will be better.

If we use short name, this pr will also be imcompatible change.

zuston · 2022-12-05T08:05:56Z

+1. Short name or concrete strategy name will be better.

If we use short name, this pr will also be incompatible change.

Yes. The refactor process will always make incompatible change. I think it could be optimized in later PRs, currently the strategies are bound to package name, it looks unreasonable.

We should reach a consensus that this way is no longer supported for new PRs. Do you think so?

By the way, I just think more when reviewing this PR.

jerqi · 2022-12-05T08:10:57Z

+1. Short name or concrete strategy name will be better.

If we use short name, this pr will also be incompatible change.

Yes. The refactor process will always make incompatible change. I think it could be optimized in later PRs, currently the strategies are bound to package name, it looks unreasonable.

We should reach a consensus that this way is no longer supported for new PRs. Do you think so?

By the way, I just think more when reviewing this PR.

I'm ok for that it is bound to package. Other systems have also similar style.

kaijchen · 2022-12-05T08:14:17Z

@smallzhongfeng is this a refactor? If so, please change [Improvement] to [Refactor] in title.

smallzhongfeng · 2022-12-05T09:02:28Z

Yes. The refactor process will always make incompatible change. I think it could be optimized in later PRs, currently the strategies are bound to package name, it looks unreasonable.

We should reach a consensus that this way is no longer supported for new PRs. Do you think so?

By the way, I just think more when reviewing this PR.

I make it final, but it doesn't work.

smallzhongfeng · 2022-12-05T09:05:26Z

move checker directory to access directory?

Em..it's ok for me actually. We can add a document to explain how migrate to higher version if we make some imcompatible changes. Or we add short name for some configs. Or we introduce the deprecated configuration option.

I will move it.
What should I do? Do I need to add a migration document?

smallzhongfeng · 2022-12-05T09:06:23Z

@smallzhongfeng is this a refactor? If so, please change [Improvement] to [Refactor] in title.

I think so. @jerqi @zuston WDYT?

jerqi · 2022-12-05T11:26:04Z

move checker directory to access directory?

Em..it's ok for me actually. We can add a document to explain how migrate to higher version if we make some imcompatible changes. Or we add short name for some configs. Or we introduce the deprecated configuration option.

I will move it.

What should I do? Do I need to add a migration document?

Yes, if you have imcompatible feature, you should add migration document, you can refer to https://github.com/apache/spark/blob/master/docs/core-migration-guide.md

smallzhongfeng · 2022-12-05T12:27:37Z

Error:  High: org.apache.uniffle.coordinator.ApplicationManager.REMOTE_PATH_SCHEMA is a mutable collection [org.apache.uniffle.coordinator.ApplicationManager] At ApplicationManager.java:[line 53] MS_MUTABLE_COLLECTION

Waht could I resolve this? I haven't met before. @zuston

What is the problem? I haven't met it before.@jerqi

jerqi · 2022-12-05T13:00:48Z

MS_MUTABLE_COLLECTION

https://blog.csdn.net/mengqingming1/article/details/106427745

jerqi · 2022-12-05T15:34:56Z

docs/coordinator-migration-guide.md

@@ -0,0 +1,25 @@
+---
+layout: page


I feel that it's enough to have an entire document. We don't need to separate it into modules, such as coordinator, server, client.

It's ok for me, updated.

The file name is still coordinator-migration-guide.md.

jerqi · 2022-12-06T06:14:37Z

LGTM, @zuston Do you have another suggestion? Let @zuston approve and merge this pr.

zuston

LGTM

zuston · 2022-12-06T07:39:15Z

Thanks @jerqi @smallzhongfeng Merged.

smallzhongfeng · 2022-12-07T04:30:12Z

Belated thanks. @jerqi @zuston

smallzhongfeng added 2 commits December 4, 2022 19:05

[Improvement] Make coordinator class more organized

7067525

pass

141cebb

jerqi requested a review from zuston December 4, 2022 11:25

pass

276230f

smallzhongfeng added 3 commits December 4, 2022 19:49

pass ut

c0f47ca

pass cs

ba3a037

pass cs

f3e869a

pass cs

6c2ac5a

smallzhongfeng closed this Dec 5, 2022

smallzhongfeng reopened this Dec 5, 2022

add coordinator-migration-guide.md

95c3b59

jerqi reviewed Dec 5, 2022

View reviewed changes

smallzhongfeng added 2 commits December 6, 2022 01:08

fix ut

da70569

pass cs

4b85fc8

pass cs

972d4ba

jerqi changed the title ~~[Improvement] Make coordinator class more organized~~ [Refactor] Make coordinator class more organized Dec 6, 2022

change name

3312b73

jerqi approved these changes Dec 6, 2022

View reviewed changes

zuston approved these changes Dec 6, 2022

View reviewed changes

zuston merged commit 8847ece into apache:master Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Make coordinator class more organized #386

[Refactor] Make coordinator class more organized #386

smallzhongfeng commented Dec 4, 2022

codecov-commenter commented Dec 4, 2022 •

edited

Loading

smallzhongfeng commented Dec 4, 2022 •

edited

Loading

zuston commented Dec 5, 2022

zuston commented Dec 5, 2022

jerqi commented Dec 5, 2022

zuston commented Dec 5, 2022

jerqi commented Dec 5, 2022

zuston commented Dec 5, 2022

jerqi commented Dec 5, 2022 •

edited

Loading

kaijchen commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

jerqi commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

jerqi commented Dec 5, 2022

jerqi Dec 5, 2022

smallzhongfeng Dec 5, 2022

jerqi Dec 6, 2022

jerqi commented Dec 6, 2022

zuston left a comment

zuston commented Dec 6, 2022

smallzhongfeng commented Dec 7, 2022

[Refactor] Make coordinator class more organized #386

[Refactor] Make coordinator class more organized #386

Conversation

smallzhongfeng commented Dec 4, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov-commenter commented Dec 4, 2022 • edited Loading

Codecov Report

smallzhongfeng commented Dec 4, 2022 • edited Loading

zuston commented Dec 5, 2022

zuston commented Dec 5, 2022

jerqi commented Dec 5, 2022

zuston commented Dec 5, 2022

jerqi commented Dec 5, 2022

zuston commented Dec 5, 2022

jerqi commented Dec 5, 2022 • edited Loading

kaijchen commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

jerqi commented Dec 5, 2022

smallzhongfeng commented Dec 5, 2022

jerqi commented Dec 5, 2022

jerqi Dec 5, 2022

Choose a reason for hiding this comment

smallzhongfeng Dec 5, 2022

Choose a reason for hiding this comment

jerqi Dec 6, 2022

Choose a reason for hiding this comment

jerqi commented Dec 6, 2022

zuston left a comment

Choose a reason for hiding this comment

zuston commented Dec 6, 2022

smallzhongfeng commented Dec 7, 2022

codecov-commenter commented Dec 4, 2022 •

edited

Loading

smallzhongfeng commented Dec 4, 2022 •

edited

Loading

jerqi commented Dec 5, 2022 •

edited

Loading