feature: https://github.com/apache/shardingsphere-elasticjob/issues/2461

[baiyangzhuhong]

Fixes #2461.

Changes proposed in this pull request:

Intent

As #2461 figured out, this post is to enhancement job strategy balance in the case of single sharding.
We want to let the job run on every job instance balanced like 'Round Robin' behavior, but not stick with only one certain instance.

Solution

Basically, the design is add new single sharding handling logic which can decouple with exists logic, and can be good benefits on expanding in the future.

So there is new SingleShardingBalanceJobShardingStrategy and added in the SPI /META-INF/services/org.apache.shardingsphere.elasticjob.kernel.internal.sharding.strategy.JobShardingStrategy list. On the other side, I do codes refactor on the job facade for the entrance of job sharding strategy action. So the JobFacade class is modified to an interface, and we have a new AbstractJobFacade abstract class, then decouple the default sharding facade with single sharding facade. I do believe making JobFacade as an interface is better, this is also a good design following 'Open-close' principle.

                                              JobFacade
                                                 |
                                                \|/
                                           AbstractJobFacade
                                                 |
                                                / \
                                            /        \
                                         /              \
                                ShardingJobFacade     SingleShardingJobFacade

SingleShardingBalanceJobShardingStrategy

Obviously, if we want to do job running balanced in distribution multiple job instance environment, we need a center controlling on the balancement status, but I don't want this behavior to be heavy. So I use ephemeral node handling in zookeeper, this can be promised that one job running on every job instance balanced is guarded in normal running. And for the other scenarios, you know, some unexpected cases, is totally fallback to the default logic. It is very lightly and effiecent.

I also add unit tests on the new classes, and modified the exists tests code.

Please do check.

Thanks

[linghengqian]

• The PR did not pass the code formatting validation. You can execute first ./mvnw spotless:apply -Pcheck -T1C. Then execute ./mvnw checkstyle:check -Pcheck -T1C to manually adjust to repair the CI.

[linghengqian]

Any updates?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 80.43478% with 27 lines in your changes missing coverage. Please review.

Project coverage is 79.47%. Comparing base (a8ba448) to head (012d7de).
Report is 56 commits behind head on master.

Files with missing lines	Patch %	Lines
...cjob/kernel/executor/facade/AbstractJobFacade.java	79.54%	8 Missing and 1 partial ⚠️
...ernel/executor/facade/SingleShardingJobFacade.java	85.45%	2 Missing and 6 partials ⚠️
...sticjob/kernel/internal/schedule/JobScheduler.java	0.00%	8 Missing ⚠️
...cjob/kernel/executor/facade/ShardingJobFacade.java	95.23%	0 Missing and 1 partial ⚠️
...type/SingleShardingBalanceJobShardingStrategy.java	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2462      +/-   ##
============================================
- Coverage     80.10%   79.47%   -0.64%     
- Complexity     1113     1123      +10     
============================================
  Files           204      207       +3     
  Lines          3821     3897      +76     
  Branches        445      465      +20     
============================================
+ Hits           3061     3097      +36     
- Misses          570      601      +31     
- Partials        190      199       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[baiyangzhuhong]

Any updates?

Thanks for your advice. I follow the points what you suggested, now all code formatting validation are passed as running:

./mvnw spotless:apply -Pcheck -T1C
./mvnw checkstyle:check -Pcheck -T1C

The new codes are pushed, please review.

But I a little confused about that the checkstyle which caused the fields of AbstractJobFacade are all changed to 'private' access ability, otherwise it would fail on checkstyle testing. This is betrayed of the original intent of AbstractJobFacade which is a parent abstract class. Seems like it doesn't make sense and a little weired.

[linghengqian]

But I a little confused about that the checkstyle which caused the fields of AbstractJobFacade are all changed to 'private' access ability, otherwise it would fail on checkstyle testing. This is betrayed of the original intent of AbstractJobFacade which is a parent abstract class. Seems like it doesn't make sense and a little weired.

• Do you mean you want to use private properties in the interface class? Can you explain why you want to do that? Interfaces should really only have public properties.

• CI is a bit shaky and I'm helping with retries.

[linghengqian]

How do I get the error in your picture? I didn't see any relevant error log in checkstyle CI.

[baiyangzhuhong]

But I a little confused about that the checkstyle which caused the fields of AbstractJobFacade are all changed to 'private' access ability, otherwise it would fail on checkstyle testing. This is betrayed of the original intent of AbstractJobFacade which is a parent abstract class. Seems like it doesn't make sense and a little weired.

• Do you mean you want to use private properties in the interface class? Can you explain why you want to do that? Interfaces should really only have public properties.

No. I am not meaning use private properties in the Interface.
What I am saying is there was checkstyle problems in the AbstractJobFacade class using 'protected' properties in my first commit codes, you know, this class is abstract class. I want the child class of AbstractJobFacade to extend the 'protected' properties. However, I have changed the codes to fix the checkstyle problems.

Never mind about that now, the current codes are still works fine.

• CI is a bit shaky and I'm helping with retries.

[baiyangzhuhong]

How do I get the error in your picture? I didn't see any relevant error log in checkstyle CI.

Never mind. The error in the picture is gone cause I had changed the codes.
Thanks.

[baiyangzhuhong]

But I a little confused about that the checkstyle which caused the fields of AbstractJobFacade are all changed to 'private' access ability, otherwise it would fail on checkstyle testing. This is betrayed of the original intent of AbstractJobFacade which is a parent abstract class. Seems like it doesn't make sense and a little weired.

• Do you mean you want to use private properties in the interface class? Can you explain why you want to do that? Interfaces should really only have public properties.

No. I am not meaning use private properties in the Interface. What I am saying is there was checkstyle problems in the AbstractJobFacade class using 'protected' properties in my first commit codes, you know, this class is abstract class. I want the child class of AbstractJobFacade to extend the 'protected' properties. However, I have changed the codes to fix the checkstyle problems.

Never mind about that now, the current codes are still works fine.

• CI is a bit shaky and I'm helping with retries.

Just for clarification,
I looked back of the checkstyle problems for first commit in the github thread, here is the original information,

you can reference the detail information from this address: https://github.com/apache/shardingsphere-elasticjob/pull/2462/checks?check_run_id=34090530003

thanks

[linghengqian]

• From my point of view, it doesn't make sense to use the protected attribute, it's better to use private directly. I don't think it's reasonable for the downstream to use the same java package as elasticjob.

• The current PR looks good from my perspective, but has some formatting issues.

• If you have anything to add, please provide your opinion.

[linghengqian]

LGTM!