narendly commented on issue #357: [WIP] Add getWorkflows(long timeout) to TaskDriver. URL: https://github.com/apache/helix/pull/357#issuecomment-514407387 1. You could leave getWorkflows() as is, and make getWorkflows(long timeout) a wrapper that calls getWorkflows() in a ExecutorService thread. This would be the only change needed for this PR; namely, adding a wrapper method with a timeout. If you want to implement a different parse logic or isWorkflowConfig method, let's create a separate issue/PR for those and discuss whether we really need them. We want to keep changes small, modular, and incremental. 2. Optional is a wrapper for its underlying objects, so it uses more memory and adds the work of boxing/unboxing. I am not claiming that this is significant, but it is a cost we do not have to incur. 3. This method could be called as frequently as users want. We do not cache at the HelixManager level and haven't had the use case for this. If there is a need, we could further discuss, but it should not be part of this PR (out of scope). -hunter On Jul 24, 2019, 00:20, at 00:20, pkuwm <[email protected]> wrote: >pkuwm commented on this pull request. > > > >> + * Batch get the configurations of all workflows in this cluster >+ * within the specified timeout in milliseconds. >+ * >+ * @param timeout a long integer presents the timeout, in >milliseconds >+ * @return a map of <String, WorkflowConfig> >+ * @throws InterruptedException if the future thread was interrupted >+ * @throws ExecutionException if the future task completed >exceptionally >+ * @throws TimeoutException if waiting for result timed out >+ */ >+ public Map<String, WorkflowConfig> getWorkflows(long timeout) >+ throws CancellationException, InterruptedException, >ExecutionException, TimeoutException { >+ if (timeout <= 0L) { >+ throw new IllegalArgumentException("timeout must be positive."); >+ } >+ >+ ExecutorService executorService = >Executors.newSingleThreadExecutor(); > >Do you have an idea of how frequently the api is called? To improve >performance, is it fine to cache the results? > >-- >You are receiving this because you commented. >Reply to this email directly or view it on GitHub: >https://github.com/apache/helix/pull/357#discussion_r306557870
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
