[GitHub] [maven-resolver] michael-o commented on a diff in pull request #239: [MRESOLVER-318] Support dynamic core count for thread config

["michael-o (via GitHub)" <gi...@apache.org>]

michael-o commented on code in PR #239:
URL: https://github.com/apache/maven-resolver/pull/239#discussion_r1098465768


##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/ExecutorUtils.java:
##########
@@ -0,0 +1,179 @@
+package org.eclipse.aether.util.concurrency;
+
+/*
+ * 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.
+ */
+
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.util.ConfigUtils;
+
+/**
+ * Utilities for executors and sizing them.
+ *
+ * @since 1.9.5
+ */
+public final class ExecutorUtils
+{
+    /**
+     * Shared instance of "direct executor".
+     */
+    public static final Executor DIRECT_EXECUTOR = Runnable::run;
+
+    /**
+     * Creates new thread pool {@link ExecutorService}. The {@code poolSize} parameter but be greater than 1.
+     */
+    public static ExecutorService threadPool( int poolSize, String namePrefix )
+    {
+        if ( poolSize < 2 )
+        {
+            throw new IllegalArgumentException(
+                    "Invalid poolSize: " + poolSize + ". Must be greater than 1." );
+        }
+        return new ThreadPoolExecutor( poolSize, poolSize, 3L, TimeUnit.SECONDS,
+                new LinkedBlockingQueue<>(),
+                new WorkerThreadFactory( namePrefix )
+        );
+    }
+
+    /**
+     * Returns {@link #DIRECT_EXECUTOR} or result of {@link #threadPool(int, String)} depending on value of
+     * {@code size} parameter.
+     */
+    public static Executor executor( int size, String namePrefix )
+    {
+        if ( size <= 1 )
+        {
+            return DIRECT_EXECUTOR;
+        }
+        else
+        {
+            return threadPool( size, namePrefix );
+        }
+    }
+
+    /**
+     * To be used with result of {@link #executor(int, String)} method, shuts down instance if it is
+     * {@link ExecutorService}.
+     */
+    public static void shutdown( Executor executor )
+    {
+        if ( executor instanceof ExecutorService )
+        {
+            ( (ExecutorService) executor ).shutdown();
+        }
+    }
+
+    /**
+     * Calculates requested thread count based on user configuration, or if none provided, the provided default value.
+     *
+     * @throws IllegalArgumentException if default value is less than 1.
+     */
+    public static int threadCount( RepositorySystemSession session, int defaultValue, String... keys )
+    {
+        if ( defaultValue < 1 )
+        {
+            throw new IllegalArgumentException(
+                    "Invalid defaultValue: " + defaultValue + ". Must be positive." );
+        }
+        String threadConfiguration = ConfigUtils.getString( session, Integer.toString( defaultValue ), keys );
+        try
+        {
+            return calculateDegreeOfConcurrency( threadConfiguration );
+        }
+        catch ( IllegalArgumentException e )
+        {
+            return defaultValue;
+        }
+    }
+
+    /**
+     * Calculates requested thread count based on user configuration, or if none provided, the provided default value.
+     * The default value is string and supports expressions like "1C".
+     *
+     * @throws IllegalArgumentException if default value is invalid.
+     */
+    public static int threadCount( RepositorySystemSession session, String defaultValue, String... keys )
+    {
+        return threadCount( session, calculateDegreeOfConcurrency( defaultValue ), keys );
+    }
+
+    /**
+     * Calculates "degree of concurrency" (count of threads to be used) based on non-null input string. String may
+     * be string representation of integer or a string representation of float followed by "C" character
+     * (case-sensitive) in which case the float is interpreted as multiplier for core count as reported by Java.
+     *
+     * Blatantly copied (and simplified) from maven-embedder
+     * {@code org.apache.maven.cli.MavenCli#calculateDegreeOfConcurrency} class.
+     */
+    private static int calculateDegreeOfConcurrency( String threadConfiguration )
+    {
+        if ( threadConfiguration == null )
+        {
+            throw new IllegalArgumentException( "Thread configuration must not be null." );

Review Comment:
   NPE



##########
src/site/markdown/configuration.md:
##########
@@ -30,7 +30,7 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
 `aether.checksums.omitChecksumsForExtensions` | String | Comma-separated list of extensions with leading dot (example `.asc`) that should have checksums omitted. These are applied to sub-artifacts only. Note: to achieve 1.7.x `aether.checksums.forSignature=true` behaviour, pass empty string as value for this property. | `.asc` | no
 `aether.checksums.algorithms` | String | Comma-separated list of checksum algorithms with which checksums are validated (downloaded) and generated (uploaded). Resolver by default supports following algorithms: `MD5`, `SHA-1`, `SHA-256` and `SHA-512`. New algorithms can be added by implementing `ChecksumAlgorithmFactory` component. | `"SHA-1,MD5"` | no
 `aether.conflictResolver.verbose` | boolean | Flag controlling the conflict resolver's verbose mode. | `false` | no
-`aether.connector.basic.threads` or `maven.artifact.threads` | int | Number of threads to use for uploading/downloading. | `5` | no
+`aether.connector.basic.threads` or `maven.artifact.threads` | String | Number of threads to use for uploading/downloading. Accepts "dynamic" expressions like `1.5C` as well, or plain integer. | `5` | no

Review Comment:
   Since this is now a string the default value should be quoted just like with other string values



##########
src/site/markdown/configuration.md:
##########
@@ -50,7 +50,7 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
 `aether.dependencyCollector.maxExceptions` | int | Only exceptions up to the number given in this configuration property are emitted. Exceptions which exceed that number are swallowed. | `50` | no
 `aether.dependencyCollector.impl` | String | The name of the dependency collector implementation to use: depth-first (original) named `df`, and breadth-first (new in 1.8.0) named `bf`. Both collectors produce equivalent results, but they may differ performance wise, depending on project being applied to. Our experience shows that existing `df` is well suited for smaller to medium size projects, while `bf` may perform better on huge projects with many dependencies. Experiment (and come back to us!) to figure out which one suits you the better. | `"df"` | no
 `aether.dependencyCollector.bf.skipper` | boolean | Flag controlling whether to skip resolving duplicate/conflicting nodes during the breadth-first (`bf`) dependency collection process. | `true` | no
-`aether.dependencyCollector.bf.threads` or `maven.artifact.threads` | int | Number of threads to use for collecting POMs and version ranges in BF collector. | `5` | no
+`aether.dependencyCollector.bf.threads` or `maven.artifact.threads` | String | Number of threads to use for collecting POMs and version ranges in BF collector. Accepts "dynamic" expressions like `1.5C` as well, or plain integer. | `5` | no

Review Comment:
   same here



##########
src/site/markdown/configuration.md:
##########
@@ -63,7 +63,7 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
 `aether.enhancedLocalRepository.releasesPrefix` | String | The prefix to use for release artifacts. | `"releases"` | no
 `aether.enhancedLocalRepository.trackingFilename` | String | Filename of the file in which to track the remote repositories. | `"_remote.repositories"` | no
 `aether.interactive` | boolean | A flag indicating whether interaction with the user is allowed. | `false` | no
-`aether.metadataResolver.threads` | int | Number of threads to use in parallel for resolving metadata. | `4` | no
+`aether.metadataResolver.threads` | String | Number of threads to use in parallel for resolving metadata. Accepts "dynamic" expressions like `1.5C` as well, or plain integer. | `4` | no

Review Comment:
   same here



-- 
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: issues-unsubscribe@maven.apache.org

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