This is an automated email from the ASF dual-hosted git repository.
jongyoul pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 7bb88cabfd [ZEPPELIN-6453] Handle secondary NotebookRepo remove
failures consistently
7bb88cabfd is described below
commit 7bb88cabfdb6bc338955ceba225ae54b1a5a8e3f
Author: DONGHOON LEE <[email protected]>
AuthorDate: Sun Jul 12 13:25:28 2026 +0900
[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 <[email protected]>
---
.../zeppelin/notebook/repo/NotebookRepoSync.java | 11 ++--
.../notebook/repo/NotebookRepoSyncTest.java | 61 ++++++++++++++++++++++
2 files changed, 69 insertions(+), 3 deletions(-)
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
index cbc2263c71..eb5b1e37e5 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
@@ -208,10 +208,15 @@ public class NotebookRepoSync implements
NotebookRepoWithVersionControl {
@Override
public void remove(String noteId, String notePath, AuthenticationInfo
subject) throws IOException {
- for (NotebookRepo repo : repos) {
- repo.remove(noteId, notePath, subject);
+ getRepo(0).remove(noteId, notePath, subject);
+ if (getRepoCount() > 1) {
+ try {
+ getRepo(1).remove(noteId, notePath, subject);
+ }
+ catch (IOException e) {
+ LOGGER.info("{}: Failed to remove from secondary storage",
e.getMessage());
+ }
}
- /* TODO(khalid): handle case when removing from secondary storage fails */
}
@Override
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java
index 539a160dcb..3a90e4f603 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java
@@ -18,12 +18,20 @@
package org.apache.zeppelin.notebook.repo;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
@@ -434,4 +442,57 @@ class NotebookRepoSyncTest {
assertEquals(0, authorizationService.getRunners(noteId).size());
assertEquals(0, authorizationService.getWriters(noteId).size());
}
+
+ @Test
+ void testRemoveSucceedsWhenSecondaryRepoFails() throws IOException {
+ NotebookRepo primaryRepo = mock(NotebookRepo.class);
+ NotebookRepo secondaryRepo = mock(NotebookRepo.class);
+ doThrow(new IOException("secondary remove failed"))
+ .when(secondaryRepo).remove("noteId", "notePath", anonymous);
+
+ try (NotebookRepoSync repoSync = newSyncWithRepos(primaryRepo,
secondaryRepo)) {
+ // secondary storage failure must not fail the whole operation
+ repoSync.remove("noteId", "notePath", anonymous);
+
+ verify(primaryRepo, times(1)).remove("noteId", "notePath", anonymous);
+ verify(secondaryRepo, times(1)).remove("noteId", "notePath", anonymous);
+ }
+ }
+
+ @Test
+ void testRemoveFailsWhenPrimaryRepoFails() throws IOException {
+ NotebookRepo primaryRepo = mock(NotebookRepo.class);
+ NotebookRepo secondaryRepo = mock(NotebookRepo.class);
+ doThrow(new IOException("primary remove failed"))
+ .when(primaryRepo).remove("noteId", "notePath", anonymous);
+
+ try (NotebookRepoSync repoSync = newSyncWithRepos(primaryRepo,
secondaryRepo)) {
+ // primary storage failure must stop the operation and propagate the
exception
+ assertThrows(IOException.class,
+ () -> repoSync.remove("noteId", "notePath", anonymous));
+
+ verify(primaryRepo, times(1)).remove("noteId", "notePath", anonymous);
+ verify(secondaryRepo, never()).remove("noteId", "notePath", anonymous);
+ }
+ }
+
+ /**
+ * Builds a NotebookRepoSync backed by the two given repos, injected through
a mocked
+ * PluginManager so the real init() path is exercised without touching
internal fields.
+ */
+ private NotebookRepoSync newSyncWithRepos(NotebookRepo primaryRepo,
NotebookRepo secondaryRepo)
+ throws IOException {
+ PluginManager mockPluginManager = mock(PluginManager.class);
+
when(mockPluginManager.loadNotebookRepo("primaryRepo")).thenReturn(primaryRepo);
+
when(mockPluginManager.loadNotebookRepo("secondaryRepo")).thenReturn(secondaryRepo);
+ // list() is queried during the init-time anonymous sync; keep it a no-op
+ when(primaryRepo.list(any())).thenReturn(new HashMap<>());
+ when(secondaryRepo.list(any())).thenReturn(new HashMap<>());
+
+ zConf.setProperty(ConfVars.ZEPPELIN_NOTEBOOK_STORAGE.getVarName(),
+ "primaryRepo,secondaryRepo");
+ NotebookRepoSync repoSync = new NotebookRepoSync(mockPluginManager);
+ repoSync.init(zConf, noteParser);
+ return repoSync;
+ }
}
