refactor parallel executor framework for junit #20910

tianhao960 · 2022-09-10T02:22:38Z

Fixes #20742.

Changes proposed in this pull request:

refactor parallel executor framework for junit
mv some implement from test-suit to test-common
use parallel executor to run test of sql parser

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 triggered maven check: mvn clean install -B -T2C -DskipTests -Dmaven.javadoc.skip=true -e.
I have made corresponding changes to the documentation.
I have added corresponding unit tests for my changes.

codecov-commenter · 2022-09-10T03:13:39Z

Codecov Report

Merging #20910 (2f3f8bf) into master (b7770e7) will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #20910      +/-   ##
============================================
+ Coverage     61.64%   61.69%   +0.04%     
- Complexity     2462     2465       +3     
============================================
  Files          4022     4034      +12     
  Lines         55560    55578      +18     
  Branches       9422     9422              
============================================
+ Hits          34249    34287      +38     
+ Misses        18429    18410      -19     
+ Partials       2882     2881       -1

Impacted Files	Coverage Δ
.../shardingsphere/infra/check/SQLCheckException.java	`0.00% <0.00%> (ø)`
...ere/infra/lock/exception/TableLockedException.java	`0.00% <0.00%> (ø)`
...fra/exception/InvalidDataNodesFormatException.java	`100.00% <0.00%> (ø)`
.../lock/exception/TableLockWaitTimeoutException.java	`0.00% <0.00%> (ø)`
...here/sql/parser/exception/SQLParsingException.java	`0.00% <0.00%> (ø)`
...exception/OverallConnectionNotEnoughException.java	`0.00% <0.00%> (ø)`
...ser/core/kernel/KernelDistSQLStatementVisitor.java	`0.00% <0.00%> (ø)`
...andler/distsql/ral/UpdatableRALBackendHandler.java	`100.00% <0.00%> (ø)`
...external/sql/type/generic/GenericSQLException.java	`0.00% <0.00%> (ø)`
...external/sql/type/wrapper/SQLWrapperException.java	`100.00% <0.00%> (ø)`
... and 144 more

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

strongduanmu

Hi @tianhao960, thank you for your great contribution. This PR is great, there are only a few minor issues that need to be optimized.

strongduanmu · 2022-09-10T23:48:38Z

...re/test/integration/framework/runner/parallel/DatabaseTypeParallelRunnerExecutorFactory.java

-import java.util.concurrent.ConcurrentHashMap;
+import org.apache.shardingsphere.test.runner.parallel.DefaultParallelRunnerExecutorFactory;
+import org.apache.shardingsphere.test.runner.parallel.ParallelRunnerExecutor;
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;

 /**
 * Parallel runner executor factory.


Please modify java doc to Database type parallel runner executor factory.

strongduanmu · 2022-09-10T23:52:07Z

...main/java/org/apache/shardingsphere/test/runner/ShardingSphereParallelTestParameterized.java

+ */
+public final class ShardingSphereParallelTestParameterized extends Parameterized {
+
+    // CHECKSTYLE:OFF


Why add CHECKSTYLE:OFF here.

check style don't allow throw Throwable and don't allow Catch Throwable, So turn off it temporary. This is follow the solution of class ShardingSphereIntegrationTestParameterized.

strongduanmu · 2022-09-10T23:52:46Z

...main/java/org/apache/shardingsphere/test/runner/ShardingSphereParallelTestParameterized.java

+        super(clazz);
+        ParallelLevel level;
+        ParallelRuntimeStrategy parallelRuntimeStrategy = clazz.getAnnotation(ParallelRuntimeStrategy.class);
+        level = null != parallelRuntimeStrategy ? parallelRuntimeStrategy.value() : ParallelLevel.DEFAULT;


Please move ParallelLevel level; here.

strongduanmu · 2022-09-10T23:53:34Z

...ava/org/apache/shardingsphere/test/runner/parallel/DefaultParallelRunnerExecutorFactory.java

+/**
+ * Parallel runner executor factory.
+ */
+public class DefaultParallelRunnerExecutorFactory<T> implements ParallelRunnerExecutorFactory<T> {


Do you think use abstract modifier for DefaultParallelRunnerExecutorFactory is better?

ShardingSphereParallelTestParameterized will create instance of DefaultParallelRunnerExecutorFactory. if make it abstract, then can't create instance

ShardingSphereParallelTestParameterized will create instance of DefaultParallelRunnerExecutorFactory. if make it abstract, then can't create instance

Ok, I got it.

strongduanmu · 2022-09-10T23:53:59Z

...ava/org/apache/shardingsphere/test/runner/parallel/DefaultParallelRunnerExecutorFactory.java

+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Parallel runner executor factory.


Default parallel runner executor factory. may be better.

strongduanmu · 2022-09-10T23:56:17Z

...test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/ParallelRunner.java

+        super(klass);
+        setScheduler(new ParallelRunnerScheduler(ParallelLevel.DEFAULT, new DefaultParallelRunnerExecutorFactory()));
+    }
+


Please remove this useless blank line.

strongduanmu · 2022-09-10T23:56:43Z

...test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/ParallelRunner.java

+import org.junit.runners.model.InitializationError;
+
+/**
+ * parallel runner for junit.


Please modify parallel to Parallel.

strongduanmu · 2022-09-10T23:58:44Z

...on/src/main/java/org/apache/shardingsphere/test/runner/parallel/annotaion/ParallelLevel.java


 /**
 * Parallel level.
 */
 public enum ParallelLevel {

-    CASE, SCENARIO
+    CASE, SCENARIO, DEFAULT


What are the scenarios where DEFAULT is used?

ParallelRunnerScheduler constructors use ParallelLevel as an input parameter;
ParallelRuntimeStrategy parallelRuntimeStrategy = clazz.getAnnotation(ParallelRuntimeStrategy.class); ParallelLevel level = null != parallelRuntimeStrategy ? parallelRuntimeStrategy.value() : ParallelLevel.DEFAULT; setScheduler(new ParallelRunnerScheduler(level, new DefaultParallelRunnerExecutorFactory()));

and I just don't want to use Case or Scenario as an default level which may be cause an confusion that why not use CaseParallelRunnerExecutor or ScenarioParallelRunnerExecutor as the executor

strongduanmu · 2022-09-10T23:59:01Z

.../java/org/apache/shardingsphere/test/runner/parallel/impl/DefaultParallelRunnerExecutor.java

+
+    @Override
+    public void execute(final T key, final Runnable childStatement) {
+


Please remove this useless blank line.

strongduanmu · 2022-09-10T23:59:21Z

.../java/org/apache/shardingsphere/test/runner/parallel/impl/DefaultParallelRunnerExecutor.java

+ * default parallel executor.
+ * @param <T> key type bind to parallel executor
+ */
+public class DefaultParallelRunnerExecutor<T> implements ParallelRunnerExecutor<T> {


Do you think add abstract for DefaultParallelRunnerExecutor is better?

DefaultParallelRunnerExecutorFactory need create instance of DefaultParallelRunnerExecutorFactory and have added an implement of method
@Override public void execute(final T key, final Runnable childStatement) { taskFeatures.add(getExecutorService(key).submit(childStatement)); }

the reason why not make DefaultParallelRunnerExecutorFactory and DefaultParallelRunnerExecutor as an abstract class is that users can just depend on test-common package , and don't depend on test-suite package

terrymanu · 2022-09-11T06:07:23Z

...main/java/org/apache/shardingsphere/test/runner/ShardingSphereParallelTestParameterized.java

+/**
+ * ShardingSphere integration test parameterized.
+ */
+public final class ShardingSphereParallelTestParameterized extends Parameterized {


The class ShardingSphereIntegrationTestParameterized had still existed.
How about consider about reuse it?

yes, this pr is based on the implement of framework of
ShardingSphereIntegrationTestParameterized. And the reason refactoring to move the implements to test-common package is that unit test cases could use this framework for parallel execution which just need to depend on test-common. And from my first thought is that ShardingSphereIntegrationTestParameterized is for integration test; while most of unit test cases don't want to depend on integration-test-suite package. and integration-test-suite could rely on test-common package. and I can't just move the framework implement from integration-test-suite package to test-common package, because the implement use some classes in integration modules and I don't want test-common package depend on it. so the basic idea of this refactor is that make an default parallel execution framework in test-common which follow the implement in integration-test-suite other than the part related with integration test, then other modules could use this framework in test-common for parallel testing . and the origin implement in integration-test-suite extend the framework in test-common for specific integration test cases.
thanks.

tianhao960 · 2022-09-13T02:05:12Z

hi, @strongduanmu @terrymanu have changed code and reply as your review suggestion. thank you.

strongduanmu

LGTM. Let's wait @terrymanu review this pr again.

terrymanu · 2022-09-14T14:34:54Z

LGTM

tianhao960 added 4 commits September 9, 2022 18:43

add support for test parallel run

ac25cb0

refactor parallel test framework apache#2

d869598

refactor parallel test framework apache#3

5891afa

refactor parallel test framework apache#4

b7a1969

strongduanmu added type: refactor in: test labels Sep 10, 2022

strongduanmu added this to the 5.2.1 milestone Sep 10, 2022

strongduanmu requested changes Sep 11, 2022

View reviewed changes

terrymanu requested changes Sep 11, 2022

View reviewed changes

tianhao960 added 2 commits September 11, 2022 22:36

refactor parallel test framework apache#5

03e083e

refactor parallel test framework apache#7

2f3f8bf

strongduanmu approved these changes Sep 14, 2022

View reviewed changes

terrymanu approved these changes Sep 14, 2022

View reviewed changes

terrymanu merged commit 541a528 into apache:master Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor parallel executor framework for junit #20910

refactor parallel executor framework for junit #20910

tianhao960 commented Sep 10, 2022 •

edited

Loading

codecov-commenter commented Sep 10, 2022 •

edited

Loading

strongduanmu left a comment

strongduanmu Sep 10, 2022

strongduanmu Sep 10, 2022

tianhao960 Sep 11, 2022

strongduanmu Sep 10, 2022

strongduanmu Sep 10, 2022

tianhao960 Sep 11, 2022 •

edited

Loading

strongduanmu Sep 14, 2022

strongduanmu Sep 10, 2022

strongduanmu Sep 10, 2022

strongduanmu Sep 10, 2022

strongduanmu Sep 10, 2022

tianhao960 Sep 11, 2022

strongduanmu Sep 10, 2022

strongduanmu Sep 10, 2022

tianhao960 Sep 11, 2022

terrymanu Sep 11, 2022

tianhao960 Sep 11, 2022

tianhao960 commented Sep 13, 2022

strongduanmu left a comment •

edited

Loading

terrymanu commented Sep 14, 2022


		@Override
		public void execute(final T key, final Runnable childStatement) {

refactor parallel executor framework for junit #20910

refactor parallel executor framework for junit #20910

Conversation

tianhao960 commented Sep 10, 2022 • edited Loading

codecov-commenter commented Sep 10, 2022 • edited Loading

Codecov Report

strongduanmu left a 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

tianhao960 Sep 11, 2022 • 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

tianhao960 commented Sep 13, 2022

strongduanmu left a comment • edited Loading

Choose a reason for hiding this comment

terrymanu commented Sep 14, 2022

tianhao960 commented Sep 10, 2022 •

edited

Loading

codecov-commenter commented Sep 10, 2022 •

edited

Loading

tianhao960 Sep 11, 2022 •

edited

Loading

strongduanmu left a comment •

edited

Loading