Skip to content

Commit 7bb88ca

Browse files
authored
[ZEPPELIN-6453] Handle secondary NotebookRepo remove failures consistently
### What is this PR for? In `NotebookRepoSync`, secondary storage failures were already handled for `save()` and `move()`, but `remove()` still used a generic `for` loop with a `TODO` comment for handling secondary remove failures. This PR makes the `remove()` failure handling consistent with the existing `save()` and `move()` behavior: * **Primary Storage (`getRepo(0)`)**: A failure during removal stops the operation and propagates the `IOException`. * **Secondary Storage (`getRepo(1)`)**: A failure during removal is caught and explicitly logged, preventing silent inconsistencies without interrupting the overall operation. To achieve this, the `for` loop in `remove(String noteId, String notePath, AuthenticationInfo subject)` was replaced with explicit `getRepo(0)` and `getRepo(1)` calls, and the outstanding `TODO` was removed. ### What type of PR is it? Improvement ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-6453 ### How should this be tested? Tested via `NotebookRepoSyncTest` using Mockito to explicitly verify the primary vs. secondary failure behaviors: * `testRemoveSucceedsWhenSecondaryRepoFails`: Verifies that an `IOException` from the secondary repo is caught and does not stop the removal process. * `testRemoveFailsWhenPrimaryRepoFails`: Verifies that an `IOException` from the primary repo halts the operation and propagates the exception. * All existing tests in `NotebookRepoSyncTest` continue to pass. ### Screenshots (if appropriate) N/A ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #5289 from move-hoon/ZEPPELIN-6453-remove-secondary-failure. Signed-off-by: Jongyoul Lee <jongyoul@gmail.com>
1 parent dfd8cf7 commit 7bb88ca

2 files changed

Lines changed: 69 additions & 3 deletions

File tree

zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -208,10 +208,15 @@ public void move(String folderPath, String newFolderPath,
208208

209209
@Override
210210
public void remove(String noteId, String notePath, AuthenticationInfo subject) throws IOException {
211-
for (NotebookRepo repo : repos) {
212-
repo.remove(noteId, notePath, subject);
211+
getRepo(0).remove(noteId, notePath, subject);
212+
if (getRepoCount() > 1) {
213+
try {
214+
getRepo(1).remove(noteId, notePath, subject);
215+
}
216+
catch (IOException e) {
217+
LOGGER.info("{}: Failed to remove from secondary storage", e.getMessage());
218+
}
213219
}
214-
/* TODO(khalid): handle case when removing from secondary storage fails */
215220
}
216221

217222
@Override

zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,20 @@
1818
package org.apache.zeppelin.notebook.repo;
1919

2020
import static org.junit.jupiter.api.Assertions.assertEquals;
21+
import static org.junit.jupiter.api.Assertions.assertThrows;
2122
import static org.junit.jupiter.api.Assertions.assertTrue;
23+
import static org.mockito.ArgumentMatchers.any;
24+
import static org.mockito.Mockito.doThrow;
2225
import static org.mockito.Mockito.mock;
26+
import static org.mockito.Mockito.never;
27+
import static org.mockito.Mockito.times;
28+
import static org.mockito.Mockito.verify;
29+
import static org.mockito.Mockito.when;
2330

2431
import java.io.File;
2532
import java.io.IOException;
2633
import java.nio.file.Files;
34+
import java.util.HashMap;
2735
import java.util.HashSet;
2836
import java.util.Map;
2937
import java.util.Set;
@@ -434,4 +442,57 @@ void testSyncWithAcl() throws IOException {
434442
assertEquals(0, authorizationService.getRunners(noteId).size());
435443
assertEquals(0, authorizationService.getWriters(noteId).size());
436444
}
445+
446+
@Test
447+
void testRemoveSucceedsWhenSecondaryRepoFails() throws IOException {
448+
NotebookRepo primaryRepo = mock(NotebookRepo.class);
449+
NotebookRepo secondaryRepo = mock(NotebookRepo.class);
450+
doThrow(new IOException("secondary remove failed"))
451+
.when(secondaryRepo).remove("noteId", "notePath", anonymous);
452+
453+
try (NotebookRepoSync repoSync = newSyncWithRepos(primaryRepo, secondaryRepo)) {
454+
// secondary storage failure must not fail the whole operation
455+
repoSync.remove("noteId", "notePath", anonymous);
456+
457+
verify(primaryRepo, times(1)).remove("noteId", "notePath", anonymous);
458+
verify(secondaryRepo, times(1)).remove("noteId", "notePath", anonymous);
459+
}
460+
}
461+
462+
@Test
463+
void testRemoveFailsWhenPrimaryRepoFails() throws IOException {
464+
NotebookRepo primaryRepo = mock(NotebookRepo.class);
465+
NotebookRepo secondaryRepo = mock(NotebookRepo.class);
466+
doThrow(new IOException("primary remove failed"))
467+
.when(primaryRepo).remove("noteId", "notePath", anonymous);
468+
469+
try (NotebookRepoSync repoSync = newSyncWithRepos(primaryRepo, secondaryRepo)) {
470+
// primary storage failure must stop the operation and propagate the exception
471+
assertThrows(IOException.class,
472+
() -> repoSync.remove("noteId", "notePath", anonymous));
473+
474+
verify(primaryRepo, times(1)).remove("noteId", "notePath", anonymous);
475+
verify(secondaryRepo, never()).remove("noteId", "notePath", anonymous);
476+
}
477+
}
478+
479+
/**
480+
* Builds a NotebookRepoSync backed by the two given repos, injected through a mocked
481+
* PluginManager so the real init() path is exercised without touching internal fields.
482+
*/
483+
private NotebookRepoSync newSyncWithRepos(NotebookRepo primaryRepo, NotebookRepo secondaryRepo)
484+
throws IOException {
485+
PluginManager mockPluginManager = mock(PluginManager.class);
486+
when(mockPluginManager.loadNotebookRepo("primaryRepo")).thenReturn(primaryRepo);
487+
when(mockPluginManager.loadNotebookRepo("secondaryRepo")).thenReturn(secondaryRepo);
488+
// list() is queried during the init-time anonymous sync; keep it a no-op
489+
when(primaryRepo.list(any())).thenReturn(new HashMap<>());
490+
when(secondaryRepo.list(any())).thenReturn(new HashMap<>());
491+
492+
zConf.setProperty(ConfVars.ZEPPELIN_NOTEBOOK_STORAGE.getVarName(),
493+
"primaryRepo,secondaryRepo");
494+
NotebookRepoSync repoSync = new NotebookRepoSync(mockPluginManager);
495+
repoSync.init(zConf, noteParser);
496+
return repoSync;
497+
}
437498
}

0 commit comments

Comments
 (0)