[GitHub] [zeppelin] Reamer commented on a diff in pull request #4382: [ZEPPELIN-5754] Init Notes in background

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

Reamer commented on code in PR #4382:
URL: https://github.com/apache/zeppelin/pull/4382#discussion_r903486737


##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/scheduler/QuartzSchedulerService.java:
##########
@@ -51,123 +51,110 @@ public class QuartzSchedulerService implements SchedulerService {
   private final ZeppelinConfiguration zeppelinConfiguration;
   private final Notebook notebook;
   private final Scheduler scheduler;
-  private final Thread loadingNotesThread;
 
   @Inject
   public QuartzSchedulerService(ZeppelinConfiguration zeppelinConfiguration, Notebook notebook)
       throws SchedulerException {
     this.zeppelinConfiguration = zeppelinConfiguration;
     this.notebook = notebook;
     this.scheduler = getScheduler();
-    this.scheduler.getListenerManager().addJobListener(new CronJobListener());
+    this.scheduler.getListenerManager().addJobListener(new MetricCronJobListener());
+    this.scheduler.getListenerManager().addTriggerListener(new ZeppelinCronJobTriggerListerner());
     this.scheduler.start();
-
-    // Do in a separated thread because there may be many notes,
-    // loop all notes in the main thread may block the restarting of Zeppelin server
-    // TODO(zjffdu) It may cause issue when user delete note before this thread is finished
-    this.loadingNotesThread = new Thread(() -> {
-        LOGGER.info("Starting init cronjobs");
-        notebook.getNotesInfo().stream()
-                .forEach(entry -> {
-                  try {
-                    refreshCron(entry.getId());
-                  } catch (Exception e) {
-                    LOGGER.warn("Fail to refresh cron for note: {}", entry.getId());
-                  }
-                });
-        LOGGER.info("Complete init cronjobs");
-    });
-    loadingNotesThread.setName("Init CronJob Thread");
-    loadingNotesThread.setDaemon(true);
-    loadingNotesThread.start();
+    // Start Init
+    notebook.addInitConsumer(this::refreshCron);
   }
 
+
   private Scheduler getScheduler() throws SchedulerException {
     return new StdSchedulerFactory().getScheduler();
   }
 
-  /**
-   * This is only for testing, unit test should always call this method in setup() before testing.
-   */
-  @VisibleForTesting
-  public void waitForFinishInit() {
+  @PreDestroy

Review Comment:
   As far as I can see, it does. We also have @PreDestroy in the Lucene sub-system to close the index cleanly and shut down the SearchService cleanly with it.
   https://github.com/apache/zeppelin/blob/5ebd0fe6231ec0b6d3576ee4ec3be14449fe5c2f/zeppelin-zengine/src/main/java/org/apache/zeppelin/search/LuceneSearch.java#L416-L430



-- 
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: dev-unsubscribe@zeppelin.apache.org

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