Skip to content

[ZEPPELIN-6600] Refresh the cron scheduler only when the expression changes #5442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
renechoi wants to merge 1 commit into
base: master
Choose a base branch
from
+53 −10
Open

[ZEPPELIN-6600] Refresh the cron scheduler only when the expression changes #5442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,15 +991,13 @@ public void updateNote(String noteId,


private boolean isCronUpdated(Map<String, Object> configA, Map<String, Object> configB) {
boolean cronUpdated = false;
if (configA.get("cron") != null && configB.get("cron") != null && configA.get("cron")
.equals(configB.get("cron"))) {
cronUpdated = true;
} else if (configA.get("cron") != null || configB.get("cron") != null) {
cronUpdated = true;
Object cronA = configA.get("cron");
Object cronB = configB.get("cron");
if (cronA == null) {
return cronB != null;
}

return cronUpdated;
return !cronA.equals(cronB);
}

public void saveNoteForms(String noteId,
Expand Down
51 changes: 48 additions & 3 deletions zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
import org.apache.zeppelin.notebook.repo.NotebookRepo;
import org.apache.zeppelin.notebook.repo.VFSNotebookRepo;
import org.apache.zeppelin.notebook.scheduler.QuartzSchedulerService;
import org.apache.zeppelin.notebook.scheduler.SchedulerService;
import org.apache.zeppelin.rest.exception.ForbiddenException;
import org.apache.zeppelin.rest.exception.NoteNotFoundException;
import org.apache.zeppelin.scheduler.Job.Status;
Expand All @@ -95,6 +96,7 @@ class NotebookServiceTest {
private SearchService searchService;
private Notebook notebook;
private AuthorizationService authorizationService;
private ZeppelinConfiguration zConf;
private ServiceContext context =
new ServiceContext(AuthenticationInfo.ANONYMOUS, new HashSet<>());

Expand All @@ -106,11 +108,12 @@ class NotebookServiceTest {
@BeforeEach
void setUp(TestInfo testInfo) throws Exception {
notebookDir = Files.createTempDirectory("notebookDir").toAbsolutePath().toFile();
ZeppelinConfiguration zConf = ZeppelinConfiguration.load();
zConf = ZeppelinConfiguration.load();
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_DIR.getVarName(),
notebookDir.getAbsolutePath());
// enable cron for testNoteUpdate method
if ("testNoteUpdate()".equals(testInfo.getDisplayName())){
// enable cron for the tests that update a note's cron settings
if ("testNoteUpdate()".equals(testInfo.getDisplayName())
|| "testCronRefreshedOnlyWhenTheExpressionChanges()".equals(testInfo.getDisplayName())) {
confDir = Files.createTempDirectory("confDir").toAbsolutePath().toFile();
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_CONF_DIR.getVarName(),
confDir.getAbsolutePath());
Expand Down Expand Up @@ -474,6 +477,48 @@ void testNoteUpdate() throws IOException {
});
}

@Test
void testCronRefreshedOnlyWhenTheExpressionChanges() throws IOException {
SchedulerService schedulerService = mock(SchedulerService.class);
NotebookService service =
new NotebookService(notebook, authorizationService, zConf, schedulerService);
String noteId = service.createNote("/folder_cron/note_test_cron", "test", true, context,
callback);

Map<String, Object> config = new HashMap<>();
config.put("isZeppelinNotebookCronEnable", true);
config.put("looknfeel", "looknfeel");
config.put("cron", "0 0/5 * * * ?");
config.put("cronExecutingRoles", "[\"test\"]");
config.put("cronExecutingUser", "test");

// adding a cron expression schedules the note
service.updateNote(noteId, "note_test_cron", new HashMap<>(config), context, callback);
verify(schedulerService).refreshCron(noteId);

// an unrelated change that keeps the same expression leaves the scheduler alone.
// onSuccess proves the update ran to the end, since updateNote has earlier exits that
// would satisfy never() without ever reaching the cron decision
reset(schedulerService);
reset(callback);
config.put("looknfeel", "simple");
service.updateNote(noteId, "note_test_cron", new HashMap<>(config), context, callback);
verify(callback).onSuccess(any(Note.class), any(ServiceContext.class));
verify(schedulerService, never()).refreshCron(noteId);

@tbonelee tbonelee Aug 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself checks out. Reverting only the production change and running this test leaves exactly one failure, at line 503, so the regression test lands on the right spot.

That one assertion, though, passes under broader conditions than intended.

updateNote has several exits before it reaches isCronUpdated (line 978): a failed checkPermission (914-917), a missing note (922), invalid settings while cron is disabled (941), and the three cron execution permission checks (953, 959, 965). None of them throw. They call callback.onFailure and return quietly, and none of them call refreshCron, so they all satisfy this never(). In other words, the assertion holds just as well when the update fails outright.

I reproduced it. Passing a copy with cronExecutingUser set to "wrong_user" at this step only makes updateNote bail at line 959 without ever reaching 978, and the test still goes green. The code this PR fixes never runs, and the build still succeeds. (Mutating the shared config map itself breaks steps 3 and 4 as well, so the sabotage has to stay local to this step for the hole to show.)

Steps 1, 3 and 4 are fine. schedulerService.refreshCron is called from a single place, line 985, which sits below 978, so the call itself proves execution got past the decision point. never() is the only assertion that needs a companion proving it got there.

callback.onSuccess at line 982 sits between isCronUpdated and refreshCron, which makes it a good companion:

reset(schedulerService);
reset(callback);
config.put("looknfeel", "simple");
service.updateNote(noteId, "note_test_cron", new HashMap<>(config), context, callback);
verify(callback).onSuccess(any(Note.class), any(ServiceContext.class));
verify(schedulerService, never()).refreshCron(noteId);

reset(callback) is needed as well, since callback is a field mock that already recorded onSuccess from createNote and from step 1. I checked that this combination fails under the sabotage above and passes on the current code. testNoteUpdate in the same file follows the same shape, resetting callback per step and verifying onSuccess.

Not a merge blocker, just a suggestion to make the regression test a bit sturdier.


// changing the expression schedules it again
reset(schedulerService);
config.put("cron", "0 0 0/1 * * ?");
service.updateNote(noteId, "note_test_cron", new HashMap<>(config), context, callback);
verify(schedulerService).refreshCron(noteId);

// removing the expression unschedules it
reset(schedulerService);
config.remove("cron");
service.updateNote(noteId, "note_test_cron", new HashMap<>(config), context, callback);
verify(schedulerService).refreshCron(noteId);
}

@Test
void testRenameNoteRejectsDuplicate() throws IOException {
String note1Id = notebookService.createNote("/folder/note1", "test", true, context, callback);
Expand Down
Loading