From 22855002c22bac900de15d181c2c1fa234b24624 Mon Sep 17 00:00:00 2001 From: Lane Li Date: Sun, 29 Sep 2024 17:48:15 +0800 Subject: [PATCH] [ZEPPELIN-6102] Fix cron disabling and refresh issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What is this PR for? 1. fix the cron disabling issue where zeppelin frontend was unable to disable cron, i.e., after setting the cron, e.g., every 1m, 5m, when you change it to "None", it didn't take effect when you refresh the notebook. 2. fixes the cron refresh issue, i.e., when you update the cron setting, it always triggered the cron with the old cron expression, because the cron setting was refreshed before notebook  updating. ### What type of PR is it? Bug Fix ### Todos ### What is the Jira issue? https://issues.apache.org/jira/projects/ZEPPELIN/issues/ZEPPELIN-6102 ### How should this be tested? 1. Open Zeppelin Web UI 2. Open any Notebook 3. Click the Cron Icon 4. set any cron expression 5. refresh the notebook and see if it worked as expected 6. set it to None 7. refresh the notebook and see if it worked as expected ### Screenshots (if appropriate) ![image](https://github.com/user-attachments/assets/503bd1d6-7c79-4ed7-8496-439244c229a8) ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #4842 from Li-GL/ZEPPELIN-6102. Signed-off-by: Cheng Pan --- .../zeppelin/service/NotebookService.java | 45 ++++++----- .../zeppelin/service/NotebookServiceTest.java | 77 ++++++++++++++++++- 2 files changed, 100 insertions(+), 22 deletions(-) diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java index dea55ac7c60..a0c8908d60e 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java @@ -898,27 +898,29 @@ public void updateNote(String noteId, return null; } } else { - AuthenticationInfo requestingAuth = new AuthenticationInfo((String)config.get("cronExecutingUser"),(String) config.get("cronExecutingRoles"), null); + if (config.get("cron") != null) { + AuthenticationInfo requestingAuth = new AuthenticationInfo((String) config.get("cronExecutingUser"), (String) config.get("cronExecutingRoles"), null); - String requestCronUser = requestingAuth.getUser(); - Set requestCronRoles = requestingAuth.getRoles(); + String requestCronUser = requestingAuth.getUser(); + Set requestCronRoles = requestingAuth.getRoles(); - if (!authorizationService.hasRunPermission(Collections.singleton(requestCronUser), note.getId())) { - LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser); - callback.onFailure(new IllegalArgumentException(requestCronUser), context); - return null; - } else { - // This part should be restarted but we need to prepare to notice who can be a cron user in advance - if (!context.getUserAndRoles().contains(requestCronUser)) { + if (!authorizationService.hasRunPermission(Collections.singleton(requestCronUser), note.getId())) { LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser); callback.onFailure(new IllegalArgumentException(requestCronUser), context); return null; - } + } else { + // This part should be restarted but we need to prepare to notice who can be a cron user in advance + if (!context.getUserAndRoles().contains(requestCronUser)) { + LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser); + callback.onFailure(new IllegalArgumentException(requestCronUser), context); + return null; + } - if (!context.getUserAndRoles().containsAll(requestCronRoles)) { - LOGGER.error("Wrong cronExecutingRoles: {}", requestCronRoles); - callback.onFailure(new IllegalArgumentException(requestCronRoles.toString()), context); - return null; + if (!context.getUserAndRoles().containsAll(requestCronRoles)) { + LOGGER.error("Wrong cronExecutingRoles: {}", requestCronRoles); + callback.onFailure(new IllegalArgumentException(requestCronRoles.toString()), context); + return null; + } } } @@ -927,17 +929,18 @@ public void updateNote(String noteId, config.remove("cron"); } } - boolean cronUpdated = isCronUpdated(config, note.getConfig()); - if (cronUpdated) { - schedulerService.refreshCron(note.getId()); - } - } + } + boolean cronUpdated = isCronUpdated(config, note.getConfig()); note.setName(name); note.setConfig(config); - notebook.updateNote(note, context.getAutheInfo()); callback.onSuccess(note, context); + // refresh cron scheduler after note update + if (cronUpdated) { + schedulerService.refreshCron(note.getId()); + } + return null; }); } diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java index 8ba1087a9f5..be30a3cda89 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java @@ -39,6 +39,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -74,6 +75,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; +import org.junit.jupiter.api.TestInfo; + import com.google.gson.Gson; @@ -82,6 +85,7 @@ class NotebookServiceTest { private static NotebookService notebookService; private File notebookDir; + private File confDir; private SearchService searchService; private Notebook notebook; private ServiceContext context = @@ -93,11 +97,21 @@ class NotebookServiceTest { @BeforeEach - void setUp() throws Exception { + void setUp(TestInfo testInfo) throws Exception { notebookDir = Files.createTempDirectory("notebookDir").toAbsolutePath().toFile(); ZeppelinConfiguration zConf = ZeppelinConfiguration.load(); zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_DIR.getVarName(), notebookDir.getAbsolutePath()); + // enable cron for testNoteUpdate method + if ("testNoteUpdate()".equals(testInfo.getDisplayName())){ + confDir = Files.createTempDirectory("confDir").toAbsolutePath().toFile(); + zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_CONF_DIR.getVarName(), + confDir.getAbsolutePath()); + zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_CRON_ENABLE.getVarName(), "true"); + String shiroPath = zConf.getAbsoluteDir(String.format("%s/shiro.ini", zConf.getConfDir())); + Files.createFile(new File(shiroPath).toPath()); + context.getUserAndRoles().add("test"); + } noteParser = new GsonNoteParser(zConf); ConfigStorage storage = ConfigStorage.createConfigStorage(zConf); NotebookRepo notebookRepo = new VFSNotebookRepo(); @@ -151,6 +165,9 @@ void setUp() throws Exception { @AfterEach void tearDown() { notebookDir.delete(); + if (confDir != null){ + confDir.delete(); + } searchService.close(); } @@ -393,6 +410,64 @@ void testNoteOperations() throws IOException { assertEquals(0, notesInfo.size()); } + @Test + void testNoteUpdate() throws IOException { + // create note + String note1Id = notebookService.createNote("/folder_update/note_test_update", "test", true, context, callback); + // update note with cron for every 5 minutes + reset(callback); + Map updatedConfig = new HashMap<>(); + updatedConfig.put("isZeppelinNotebookCronEnable", true); + updatedConfig.put("looknfeel", "looknfeel"); + updatedConfig.put("personalizedMode", "false"); + updatedConfig.put("cron", "0 0/5 * * * ?"); + updatedConfig.put("cronExecutingRoles", "[\"test\"]"); + updatedConfig.put("cronExecutingUser", "test"); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + assertEquals("note_test_update", note1.getName()); + assertEquals("test", note1.getConfig().get("cronExecutingUser")); + assertEquals("[\"test\"]", note1.getConfig().get("cronExecutingRoles")); + assertEquals("0 0/5 * * * ?", note1.getConfig().get("cron")); + verify(callback).onSuccess(any(Note.class), any(ServiceContext.class)); + return null; + }); + // update note with cron for every 1 hour + reset(callback); + updatedConfig.put("cron", "0 0 0/1 * * ?"); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + assertEquals("0 0 0/1 * * ?", note1.getConfig().get("cron")); + verify(callback).onSuccess(any(Note.class), any(ServiceContext.class)); + return null; + }); + // update note with wrong user + reset(callback); + updatedConfig.put("cronExecutingUser", "wrong_user"); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + verify(callback).onFailure(any(IllegalArgumentException.class), any(ServiceContext.class)); + return null; + }); + // disable cron + reset(callback); + updatedConfig.put("cron", null); + updatedConfig.put("cronExecutingRoles", ""); + updatedConfig.put("cronExecutingUser", ""); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + assertNull(note1.getConfig().get("cron")); + assertEquals("", note1.getConfig().get("cronExecutingRoles")); + assertEquals("", note1.getConfig().get("cronExecutingUser")); + verify(callback).onSuccess(any(Note.class), any(ServiceContext.class)); + return null; + }); + } + @Test void testRenameNoteRejectsDuplicate() throws IOException { String note1Id = notebookService.createNote("/folder/note1", "test", true, context, callback);