[jira] [Commented] (DRILL-7223) Make the timeout in TimedCallable a configurable boot time parameter

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/DRILL-7223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16831373#comment-16831373 ] 

ASF GitHub Bot commented on DRILL-7223:
---------------------------------------

amansinha100 commented on pull request #1776: DRILL-7223: Create an option to control timeout for REFRESH METADATA
URL: https://github.com/apache/drill/pull/1776#discussion_r280268645
 
 

 ##########
 File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/TimedCallable.java
 ##########
 @@ -188,6 +187,23 @@ private long getExecutionTime(TimeUnit unit) {
     return unit.convert(executionTime, TimeUnit.NANOSECONDS);
   }
 
+  /**
+   * Execute the list of runnables with the given parallelization.  At end, return values and report completion time
+   * stats to provided logger. Each runnable is allowed a certain timeout. If the timeout exceeds, existing/pending
+   * tasks will be cancelled and a {@link UserException} is thrown.
+   * @param activity Name of activity for reporting in logger.
+   * @param logger The logger to use to report results.
+   * @param tasks List of callable that should be executed and timed.  If this list has one item, task will be
+   *                  completed in-thread. Each callable must handle {@link InterruptedException}s.
+   * @param parallelism  The number of threads that should be run to complete this task.
+   * @param timeout if bigger than zero, set the timeout per runnable (in msec)
+   * @return The list of outcome objects.
+   * @throws IOException All exceptions are coerced to IOException since this was build for storage system tasks initially.
+   */
+  public static <V> List<V> run(final String activity, final Logger logger, final List<TimedCallable<V>> tasks, int parallelism, long timeout) throws IOException {
+    TIMEOUT_PER_RUNNABLE_IN_MSECS = timeout;
 
 Review comment:
   By convention we treat these as static final constants (although I see that the `final` is missing).  Also assigning to this static variable is not thread safe. Actually, now that this is a config option, why do we need a static constant ?  Why not just pass it as a parameter wherever needed including passing it to the constructor of classes that are extending the abstract class and then we can set the timeout as a local class variable ?
 
----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Make the timeout in TimedCallable a configurable boot time parameter
> --------------------------------------------------------------------
>
>                 Key: DRILL-7223
>                 URL: https://issues.apache.org/jira/browse/DRILL-7223
>             Project: Apache Drill
>          Issue Type: Improvement
>    Affects Versions: 1.16.0
>            Reporter: Aman Sinha
>            Assignee: Boaz Ben-Zvi
>            Priority: Minor
>             Fix For: 1.17.0
>
>
> The [TimedCallable.TIMEOUT_PER_RUNNABLE_IN_MSECS|https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/store/TimedCallable.java#L52] is currently an internal Drill constant defined as 15 secs. This has been there from day 1 of the introduction. Drill's TimedCallable implements the Java concurrency's Callable interface to create timed threads. It is used by the REFRESH METADATA command which creates multiple threads on the Foreman node to gather Parquet metadata to build the metadata cache.
> Depending on the load on the system or for very large scale number of parquet files (millions) it is possible to exceed this timeout.  While the exact root cause of exceeding the timeout is being investigated, it makes sense to make this timeout a configurable parameter to aid with large scale testing. This JIRA is to make this a configurable bootstrapping option in the drill-override.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)