Skip to content

Commit 2285500

Browse files
Li-GLLi-GL
authored
[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) ![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 <[email protected]>
1 parent b9ff26f commit 2285500

File tree

2 files changed

+100
-22
lines changed

2 files changed

+100
-22
lines changed

‎zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -898,27 +898,29 @@ public void updateNote(String noteId,
898898
return null;
899899
}
900900
} else {
901-
AuthenticationInfo requestingAuth = new AuthenticationInfo((String)config.get("cronExecutingUser"),(String) config.get("cronExecutingRoles"), null);
901+
if (config.get("cron") != null) {
902+
AuthenticationInfo requestingAuth = new AuthenticationInfo((String) config.get("cronExecutingUser"), (String) config.get("cronExecutingRoles"), null);
902903

903-
String requestCronUser = requestingAuth.getUser();
904-
Set<String> requestCronRoles = requestingAuth.getRoles();
904+
String requestCronUser = requestingAuth.getUser();
905+
Set<String> requestCronRoles = requestingAuth.getRoles();
905906

906-
if (!authorizationService.hasRunPermission(Collections.singleton(requestCronUser), note.getId())) {
907-
LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser);
908-
callback.onFailure(new IllegalArgumentException(requestCronUser), context);
909-
return null;
910-
} else {
911-
// This part should be restarted but we need to prepare to notice who can be a cron user in advance
912-
if (!context.getUserAndRoles().contains(requestCronUser)) {
907+
if (!authorizationService.hasRunPermission(Collections.singleton(requestCronUser), note.getId())) {
913908
LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser);
914909
callback.onFailure(new IllegalArgumentException(requestCronUser), context);
915910
return null;
916-
}
911+
} else {
912+
// This part should be restarted but we need to prepare to notice who can be a cron user in advance
913+
if (!context.getUserAndRoles().contains(requestCronUser)) {
914+
LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser);
915+
callback.onFailure(new IllegalArgumentException(requestCronUser), context);
916+
return null;
917+
}
917918

918-
if (!context.getUserAndRoles().containsAll(requestCronRoles)) {
919-
LOGGER.error("Wrong cronExecutingRoles: {}", requestCronRoles);
920-
callback.onFailure(new IllegalArgumentException(requestCronRoles.toString()), context);
921-
return null;
919+
if (!context.getUserAndRoles().containsAll(requestCronRoles)) {
920+
LOGGER.error("Wrong cronExecutingRoles: {}", requestCronRoles);
921+
callback.onFailure(new IllegalArgumentException(requestCronRoles.toString()), context);
922+
return null;
923+
}
922924
}
923925
}
924926

@@ -927,17 +929,18 @@ public void updateNote(String noteId,
927929
config.remove("cron");
928930
}
929931
}
930-
boolean cronUpdated = isCronUpdated(config, note.getConfig());
931-
if (cronUpdated) {
932-
schedulerService.refreshCron(note.getId());
933-
}
934-
}
935932

933+
}
934+
boolean cronUpdated = isCronUpdated(config, note.getConfig());
936935
note.setName(name);
937936
note.setConfig(config);
938-
939937
notebook.updateNote(note, context.getAutheInfo());
940938
callback.onSuccess(note, context);
939+
// refresh cron scheduler after note update
940+
if (cronUpdated) {
941+
schedulerService.refreshCron(note.getId());
942+
}
943+
941944
return null;
942945
});
943946
}

‎zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import java.util.HashMap;
4040
import java.util.HashSet;
4141
import java.util.List;
42+
import java.util.Map;
4243
import java.util.concurrent.TimeUnit;
4344
import java.util.stream.Collectors;
4445
import java.util.stream.IntStream;
@@ -74,6 +75,8 @@
7475
import org.junit.jupiter.api.BeforeEach;
7576
import org.junit.jupiter.api.Test;
7677
import org.mockito.ArgumentCaptor;
78+
import org.junit.jupiter.api.TestInfo;
79+
7780

7881
import com.google.gson.Gson;
7982

@@ -82,6 +85,7 @@ class NotebookServiceTest {
8285
private static NotebookService notebookService;
8386

8487
private File notebookDir;
88+
private File confDir;
8589
private SearchService searchService;
8690
private Notebook notebook;
8791
private ServiceContext context =
@@ -93,11 +97,21 @@ class NotebookServiceTest {
9397

9498

9599
@BeforeEach
96-
void setUp() throws Exception {
100+
void setUp(TestInfo testInfo) throws Exception {
97101
notebookDir = Files.createTempDirectory("notebookDir").toAbsolutePath().toFile();
98102
ZeppelinConfiguration zConf = ZeppelinConfiguration.load();
99103
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_DIR.getVarName(),
100104
notebookDir.getAbsolutePath());
105+
// enable cron for testNoteUpdate method
106+
if ("testNoteUpdate()".equals(testInfo.getDisplayName())){
107+
confDir = Files.createTempDirectory("confDir").toAbsolutePath().toFile();
108+
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_CONF_DIR.getVarName(),
109+
confDir.getAbsolutePath());
110+
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_CRON_ENABLE.getVarName(), "true");
111+
String shiroPath = zConf.getAbsoluteDir(String.format("%s/shiro.ini", zConf.getConfDir()));
112+
Files.createFile(new File(shiroPath).toPath());
113+
context.getUserAndRoles().add("test");
114+
}
101115
noteParser = new GsonNoteParser(zConf);
102116
ConfigStorage storage = ConfigStorage.createConfigStorage(zConf);
103117
NotebookRepo notebookRepo = new VFSNotebookRepo();
@@ -151,6 +165,9 @@ void setUp() throws Exception {
151165
@AfterEach
152166
void tearDown() {
153167
notebookDir.delete();
168+
if (confDir != null){
169+
confDir.delete();
170+
}
154171
searchService.close();
155172
}
156173

@@ -393,6 +410,64 @@ void testNoteOperations() throws IOException {
393410
assertEquals(0, notesInfo.size());
394411
}
395412

413+
@Test
414+
void testNoteUpdate() throws IOException {
415+
// create note
416+
String note1Id = notebookService.createNote("/folder_update/note_test_update", "test", true, context, callback);
417+
// update note with cron for every 5 minutes
418+
reset(callback);
419+
Map<String, Object> updatedConfig = new HashMap<>();
420+
updatedConfig.put("isZeppelinNotebookCronEnable", true);
421+
updatedConfig.put("looknfeel", "looknfeel");
422+
updatedConfig.put("personalizedMode", "false");
423+
updatedConfig.put("cron", "0 0/5 * * * ?");
424+
updatedConfig.put("cronExecutingRoles", "[\"test\"]");
425+
updatedConfig.put("cronExecutingUser", "test");
426+
notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback);
427+
notebook.processNote(note1Id,
428+
note1 -> {
429+
assertEquals("note_test_update", note1.getName());
430+
assertEquals("test", note1.getConfig().get("cronExecutingUser"));
431+
assertEquals("[\"test\"]", note1.getConfig().get("cronExecutingRoles"));
432+
assertEquals("0 0/5 * * * ?", note1.getConfig().get("cron"));
433+
verify(callback).onSuccess(any(Note.class), any(ServiceContext.class));
434+
return null;
435+
});
436+
// update note with cron for every 1 hour
437+
reset(callback);
438+
updatedConfig.put("cron", "0 0 0/1 * * ?");
439+
notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback);
440+
notebook.processNote(note1Id,
441+
note1 -> {
442+
assertEquals("0 0 0/1 * * ?", note1.getConfig().get("cron"));
443+
verify(callback).onSuccess(any(Note.class), any(ServiceContext.class));
444+
return null;
445+
});
446+
// update note with wrong user
447+
reset(callback);
448+
updatedConfig.put("cronExecutingUser", "wrong_user");
449+
notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback);
450+
notebook.processNote(note1Id,
451+
note1 -> {
452+
verify(callback).onFailure(any(IllegalArgumentException.class), any(ServiceContext.class));
453+
return null;
454+
});
455+
// disable cron
456+
reset(callback);
457+
updatedConfig.put("cron", null);
458+
updatedConfig.put("cronExecutingRoles", "");
459+
updatedConfig.put("cronExecutingUser", "");
460+
notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback);
461+
notebook.processNote(note1Id,
462+
note1 -> {
463+
assertNull(note1.getConfig().get("cron"));
464+
assertEquals("", note1.getConfig().get("cronExecutingRoles"));
465+
assertEquals("", note1.getConfig().get("cronExecutingUser"));
466+
verify(callback).onSuccess(any(Note.class), any(ServiceContext.class));
467+
return null;
468+
});
469+
}
470+
396471
@Test
397472
void testRenameNoteRejectsDuplicate() throws IOException {
398473
String note1Id = notebookService.createNote("/folder/note1", "test", true, context, callback);

0 commit comments

Comments
 (0)