This is an automated email from the ASF dual-hosted git repository.
chengpan pushed a commit to branch branch-0.12
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/branch-0.12 by this push:
new da185535b2 [ZEPPELIN-6102] Fix cron disabling and refresh issue
da185535b2 is described below
commit da185535b2d7fd1b3ffeb2e7c1235b1a4a5aa3c9
Author: Lane Li <lglfa...@gmail.com>
AuthorDate: Sun Sep 29 17:48:15 2024 +0800
[ZEPPELIN-6102] Fix cron disabling and refresh issue
### 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)
### 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 <cheng...@apache.org>
(cherry picked from commit 22855002c22bac900de15d181c2c1fa234b24624)
Signed-off-by: Cheng Pan <cheng...@apache.org>
---
.../apache/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 dea55ac7c6..a0c8908d60 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 class NotebookService {
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<String> requestCronRoles = requestingAuth.getRoles();
+ String requestCronUser = requestingAuth.getUser();
+ Set<String> 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 class NotebookService {
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 8ba1087a9f..be30a3cda8 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.nio.file.Files;
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.AfterEach;
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 @@ class NotebookServiceTest {
@AfterEach
void tearDown() {
notebookDir.delete();
+ if (confDir != null){
+ confDir.delete();
+ }
searchService.close();
}
@@ -393,6 +410,64 @@ class NotebookServiceTest {
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<String, Object> 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);
