phet commented on code in PR #3879:
URL: https://github.com/apache/gobblin/pull/3879#discussion_r1498166313
##########
gobblin-utility/src/main/java/org/apache/gobblin/util/ParallelRunner.java:
##########
@@ -86,6 +87,10 @@ public class ParallelRunner implements Closeable {
public static final String PARALLEL_RUNNER_THREADS_KEY =
"parallel.runner.threads";
public static final int DEFAULT_PARALLEL_RUNNER_THREADS = 10;
+ public static int readConfigNumParallelRunnerThreads(Properties props) {
+ return Integer.parseInt(props.getProperty(PARALLEL_RUNNER_THREADS_KEY,
Integer.toString(DEFAULT_PARALLEL_RUNNER_THREADS)));
+ }
+
Review Comment:
the primary reason I moved it here is that `MRJobRunner` [was referencing
two
configs](https://github.com/apache/gobblin/pull/3879/files#diff-4be3972775f1f53766134530e51058f1efbcb527dae787b37225865dd529c8bbL258)
both defined in this class.
we can certainly rename. I'd prefer not "default", just because the
*default value* is only used as a fallback, in the case of no specific config.
as for multiple classes all leveraging this utility, while wanting their own
setting, I recommend hierarchical naming, a la typesafe `Config`s -
```
Sting thisClassConfigPrefix = getClass().getName(); // potential
convention...
Config thisClassConfigs = ConfigUtils.getConfig(myConfig,
thisClassConfigPrefix, ConfigFactory.empty());
int numThreads = ParallelRunner.getNumThreadsConfig(thisClassConfigs);
```
at the moment I'd continue using `Properties`, since my objective was just
to consolidate cross-class references... but if we find more widespread use, we
could update to take `Config` instead.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
