[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #20910: refactor parallel executor framework for junit

[GitBox <gi...@apache.org>]

strongduanmu commented on code in PR #20910:
URL: https://github.com/apache/shardingsphere/pull/20910#discussion_r967716798


##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/DefaultParallelRunnerExecutorFactory.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel;
+
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.apache.shardingsphere.test.runner.parallel.impl.DefaultParallelRunnerExecutor;
+
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Parallel runner executor factory.
+ */
+public class DefaultParallelRunnerExecutorFactory<T> implements ParallelRunnerExecutorFactory<T> {

Review Comment:
   Do you think use abstract modifier for DefaultParallelRunnerExecutorFactory is better?



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/impl/DefaultParallelRunnerExecutor.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel.impl;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import lombok.Getter;
+import org.apache.shardingsphere.test.runner.parallel.ParallelRunnerExecutor;
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+/**
+ * default parallel executor.
+ * @param <T> key type bind to parallel executor
+ */
+public class DefaultParallelRunnerExecutor<T> implements ParallelRunnerExecutor<T> {
+    
+    @Getter
+    private final Collection<Future<?>> taskFeatures = new LinkedList<>();
+    
+    private final ExecutorService executorService;
+    
+    public DefaultParallelRunnerExecutor() {
+        executorService = Executors.newFixedThreadPool(
+                Runtime.getRuntime().availableProcessors(),
+                new ThreadFactoryBuilder().setDaemon(true).setNameFormat("ShardingSphere-ParallelTestThread-%d").build());
+    }
+    
+    @Override
+    public void execute(final T key, final Runnable childStatement) {
+        

Review Comment:
   Please remove this useless blank line.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/annotaion/ParallelLevel.java:
##########
@@ -15,12 +15,12 @@
  * limitations under the License.
  */
 
-package org.apache.shardingsphere.test.integration.framework.runner.parallel.annotaion;
+package org.apache.shardingsphere.test.runner.parallel.annotaion;
 
 /**
  * Parallel level.
  */
 public enum ParallelLevel {
     
-    CASE, SCENARIO
+    CASE, SCENARIO, DEFAULT

Review Comment:
   What are the scenarios where DEFAULT is used?



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/DefaultParallelRunnerExecutorFactory.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel;
+
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.apache.shardingsphere.test.runner.parallel.impl.DefaultParallelRunnerExecutor;
+
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Parallel runner executor factory.

Review Comment:
   `Default parallel runner executor factory.` may be better.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/DefaultParallelRunnerExecutorFactory.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel;
+
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.apache.shardingsphere.test.runner.parallel.impl.DefaultParallelRunnerExecutor;
+
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Parallel runner executor factory.
+ */
+public class DefaultParallelRunnerExecutorFactory<T> implements ParallelRunnerExecutorFactory<T> {
+    
+    private final Map<T, ParallelRunnerExecutor> executors = new ConcurrentHashMap<>();
+    
+    private volatile ParallelRunnerExecutor defaultExecutor;
+    
+    @Override
+    public ParallelRunnerExecutor getExecutor(final T key, final ParallelLevel parallelLevel) {
+        if (executors.containsKey(key)) {
+            return executors.get(key);
+        }
+        ParallelRunnerExecutor newExecutor = newInstance(parallelLevel);
+        if (null != executors.putIfAbsent(key, newExecutor)) {
+            newExecutor.finished();
+        }
+        return executors.get(key);
+    }
+    
+    /**
+     * Get parallel runner executor.
+     *
+     * @param parallelLevel parallel level
+     * @return parallel runner executor
+     */
+    public ParallelRunnerExecutor getExecutor(final ParallelLevel parallelLevel) {
+        if (null == defaultExecutor) {
+            synchronized (ParallelRunnerExecutor.class) {
+                if (null == defaultExecutor) {
+                    defaultExecutor = new DefaultParallelRunnerExecutor();
+                }
+            }
+        }
+        return defaultExecutor;
+    }
+    
+    /**
+     * create executor instance by parallel level.
+     * @param parallelLevel parallel level
+     * @return executor by parallel level
+     */
+    public ParallelRunnerExecutor newInstance(final ParallelLevel parallelLevel) {
+        return new DefaultParallelRunnerExecutor();
+    }
+    
+    /**
+     * Get all executors.
+     *
+     * @return all executors
+     */
+    public Collection<ParallelRunnerExecutor> getAllExecutors() {
+        List<ParallelRunnerExecutor> executors = new LinkedList<>(this.executors.values());
+        if (null != defaultExecutor) {
+            executors.add(defaultExecutor);
+        }
+        return executors;
+    }
+    

Review Comment:
   Please remove this useless blank line.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/ParallelRunner.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel;
+
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.junit.runners.BlockJUnit4ClassRunner;
+import org.junit.runners.model.InitializationError;
+
+/**
+ * parallel runner for junit.

Review Comment:
   Please modify `parallel` to `Parallel`.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/ParallelRunner.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel;
+
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.junit.runners.BlockJUnit4ClassRunner;
+import org.junit.runners.model.InitializationError;
+
+/**
+ * parallel runner for junit.
+ */
+public class ParallelRunner extends BlockJUnit4ClassRunner {
+    
+    /**
+     * Creates a ParallelRunner to run {@code klass}.
+     * If you now annotate a test-class with @RunWith(ParallelRunner.class) each method will run within its own thread.
+     *
+     * @param klass test class
+     * @throws InitializationError if the test class is malformed.
+     */
+    public ParallelRunner(final Class<?> klass) throws InitializationError {
+        super(klass);
+        setScheduler(new ParallelRunnerScheduler(ParallelLevel.DEFAULT, new DefaultParallelRunnerExecutorFactory()));
+    }
+    

Review Comment:
   Please remove this useless blank line.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/ShardingSphereParallelTestParameterized.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner;
+
+import org.apache.shardingsphere.test.runner.parallel.DefaultParallelRunnerExecutorFactory;
+import org.apache.shardingsphere.test.runner.parallel.ParallelRunnerScheduler;
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelRuntimeStrategy;
+import org.junit.runners.Parameterized;
+
+/**
+ * ShardingSphere integration test parameterized.
+ */
+public final class ShardingSphereParallelTestParameterized extends Parameterized {
+    
+    // CHECKSTYLE:OFF
+    public ShardingSphereParallelTestParameterized(final Class<?> clazz) throws Throwable {
+        // CHECKSTYLE:ON
+        super(clazz);
+        ParallelLevel level;
+        ParallelRuntimeStrategy parallelRuntimeStrategy = clazz.getAnnotation(ParallelRuntimeStrategy.class);
+        level = null != parallelRuntimeStrategy ? parallelRuntimeStrategy.value() : ParallelLevel.DEFAULT;

Review Comment:
   Please move `ParallelLevel level;` here.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/ShardingSphereParallelTestParameterized.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner;
+
+import org.apache.shardingsphere.test.runner.parallel.DefaultParallelRunnerExecutorFactory;
+import org.apache.shardingsphere.test.runner.parallel.ParallelRunnerScheduler;
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelRuntimeStrategy;
+import org.junit.runners.Parameterized;
+
+/**
+ * ShardingSphere integration test parameterized.
+ */
+public final class ShardingSphereParallelTestParameterized extends Parameterized {
+    
+    // CHECKSTYLE:OFF

Review Comment:
   Why add CHECKSTYLE:OFF here.



##########
shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-suite/src/test/java/org/apache/shardingsphere/test/integration/framework/runner/parallel/DatabaseTypeParallelRunnerExecutorFactory.java:
##########
@@ -18,40 +18,19 @@
 package org.apache.shardingsphere.test.integration.framework.runner.parallel;
 
 import org.apache.shardingsphere.infra.database.type.DatabaseType;
-import org.apache.shardingsphere.test.integration.framework.runner.parallel.annotaion.ParallelLevel;
 import org.apache.shardingsphere.test.integration.framework.runner.parallel.impl.CaseParallelRunnerExecutor;
 import org.apache.shardingsphere.test.integration.framework.runner.parallel.impl.ScenarioParallelRunnerExecutor;
-
-import java.util.Collection;
-import java.util.Map;
-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.

Review Comment:
   Please modify java doc to `Database type parallel runner executor factory.`



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/DefaultParallelRunnerExecutorFactory.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel;
+
+import org.apache.shardingsphere.test.runner.parallel.annotaion.ParallelLevel;
+import org.apache.shardingsphere.test.runner.parallel.impl.DefaultParallelRunnerExecutor;
+
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Parallel runner executor factory.
+ */
+public class DefaultParallelRunnerExecutorFactory<T> implements ParallelRunnerExecutorFactory<T> {
+    
+    private final Map<T, ParallelRunnerExecutor> executors = new ConcurrentHashMap<>();
+    
+    private volatile ParallelRunnerExecutor defaultExecutor;
+    
+    @Override
+    public ParallelRunnerExecutor getExecutor(final T key, final ParallelLevel parallelLevel) {
+        if (executors.containsKey(key)) {
+            return executors.get(key);
+        }
+        ParallelRunnerExecutor newExecutor = newInstance(parallelLevel);
+        if (null != executors.putIfAbsent(key, newExecutor)) {
+            newExecutor.finished();
+        }
+        return executors.get(key);
+    }
+    
+    /**
+     * Get parallel runner executor.
+     *
+     * @param parallelLevel parallel level
+     * @return parallel runner executor
+     */
+    public ParallelRunnerExecutor getExecutor(final ParallelLevel parallelLevel) {
+        if (null == defaultExecutor) {
+            synchronized (ParallelRunnerExecutor.class) {
+                if (null == defaultExecutor) {
+                    defaultExecutor = new DefaultParallelRunnerExecutor();
+                }
+            }
+        }
+        return defaultExecutor;
+    }
+    
+    /**
+     * create executor instance by parallel level.

Review Comment:
   Please modify create to Create, and add a blank line after this line.



##########
shardingsphere-test/shardingsphere-test-common/src/main/java/org/apache/shardingsphere/test/runner/parallel/impl/DefaultParallelRunnerExecutor.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.test.runner.parallel.impl;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import lombok.Getter;
+import org.apache.shardingsphere.test.runner.parallel.ParallelRunnerExecutor;
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+/**
+ * default parallel executor.
+ * @param <T> key type bind to parallel executor
+ */
+public class DefaultParallelRunnerExecutor<T> implements ParallelRunnerExecutor<T> {

Review Comment:
   Do you think add abstract for DefaultParallelRunnerExecutor is better?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org