Add async deletion - #198
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds directory element status transitions, a managed asynchronous execution service, and asynchronous single and bulk deletion flows. Tests add WireMock-based HTTP stubbing and verification for background deletion behavior. ChangesDirectory deletion lifecycle
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExploreService
participant ExploreServerExecutionService
participant DirectoryService
participant DirectoryServer
Caller->>ExploreService: deleteElement(...) or deleteElementsFromDirectory(...)
ExploreService->>ExploreServerExecutionService: runAsync(deletion task)
ExploreServerExecutionService->>ExploreService: execute deletion task
ExploreService->>DirectoryService: updateElementsStatus(..., DELETING, userId)
ExploreService->>DirectoryServer: delete directory elements
ExploreService->>DirectoryService: updateElementsStatus(..., ACTIVE, userId) for failures
ExploreServerExecutionService-->>Caller: CompletableFuture completion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| @PostConstruct | ||
| private void postConstruct() { | ||
| ContextSnapshotFactory snapshotFactory = ContextSnapshotFactory.builder().build(); | ||
| executorService = ContextExecutorService.wrap(Executors.newCachedThreadPool(), |
There was a problem hiding this comment.
I don't think it's crucial to have a thread limit with this implementation, it spawns only one thread per deletion request. Previously I tried doing n thread for n element to delete and added a thread count limit but its potential to suck up all resources was too risky
| try { | ||
| directoryService.deleteElement(id, userId); | ||
| directoryService.deleteDirectoryElement(id, userId); | ||
| // FIXME dirty fix to ignore errors and still delete the elements in the directory-server. To delete when handled properly. |
There was a problem hiding this comment.
I don't understand this part, why this fixme and the dev didn't do a finally ?
There was a problem hiding this comment.
it's legacy, there was a period when we struggled with elements still appearing in the directory even after a deletion so a dirty overkill solution was adopted. Not mutualizing deleteDirectoryElement in a finally clause was surely an oversight
There was a problem hiding this comment.
That said I don't see drawbacks to moving it to a finally in this PR
| } finally { | ||
| directoryService.deleteElementsFromDirectory(uuids, parentDirectoryUuids, userId); | ||
| } | ||
| return exploreServerExecutionService.runAsync(() -> { |
There was a problem hiding this comment.
By the way, why the asynchronous threads are not inside directory server instead ?
There was a problem hiding this comment.
deletion is all orchestrated by explore server, directory server has no vision over all the servers holding the actual data
There was a problem hiding this comment.
maybe the logic should be separated from the directoryService to make this clearer
| directoryService.deleteElement(id, userId); | ||
| deletedIds.add(id); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Failed to delete element {}", id, e); |
There was a problem hiding this comment.
So without a finally, the status can be stuck in DELETING forever no ?
There was a problem hiding this comment.
Same if a thread stop while executing.
There was a problem hiding this comment.
I've added a tracking of failed deletion in order to revert those back.
For thread crash it's more tricky and would necessitate a more global solution at the project level to establish how we handle these type of recovery imo
|
|
||
| public void deleteElementsFromDirectory(List<UUID> uuids, UUID parentDirectoryUuids, String userId) { | ||
| public CompletableFuture<Void> deleteElementsFromDirectory(List<UUID> uuids, UUID parentDirectoryUuid, String userId) { | ||
| directoryService.updateElementsStatus(uuids, "DELETING", userId); |
There was a problem hiding this comment.
Why it's outside the async ?
There was a problem hiding this comment.
No valid reason, I focused on turning the deletion async but this can also be included in the new thread
There was a problem hiding this comment.
Just added an enum to make the call cleaner btw
| return new MockResponse(200); | ||
| } else if (path.matches("/v1/elements\\?ids=.*&status=.*") && "PUT".equals(request.getMethod())) { |
There was a problem hiding this comment.
I though every new test should be with WireMock ?
There was a problem hiding this comment.
True I added mindlessly added it here since there's currently no wiremock test in this class but let's do things clean
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/explore/server/ExploreTest.java (1)
180-181: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
WireMockServeris started/stopped for every test, not just the one that needs it.Only
testDeleteElementSpawnsDeletionThreaduseswireMockServer, yet it's instantiated and started in the class-wide@BeforeEachand stopped in@AfterEach, adding a real socket bind/teardown to every other test in this large class.♻️ Proposed refactor to scope WireMockServer to the single test
- private WireMockServer wireMockServer; - ... `@BeforeEach` void setup(final MockWebServer server) throws Exception { - wireMockServer = new WireMockServer(wireMockConfig().dynamicPort()); - wireMockServer.start(); - ... } - `@AfterEach` - void teardown() { - if (wireMockServer != null) { - wireMockServer.stop(); - } - } - `@Test` void testDeleteElementSpawnsDeletionThread() throws Exception { + WireMockServer wireMockServer = new WireMockServer(wireMockConfig().dynamicPort()); + wireMockServer.start(); + try { ... + } finally { + wireMockServer.stop(); + } }Also applies to: 193-195, 522-528
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/explore/server/ExploreTest.java` around lines 180 - 181, Scope the WireMockServer lifecycle to testDeleteElementSpawnsDeletionThread instead of the class-wide setup. Remove its `@BeforeEach` initialization and `@AfterEach` shutdown, instantiate and start it within that test, and stop it when the test completes while preserving the existing wireMockServer stubbing and usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/gridsuite/explore/server/services/ExploreServerExecutionService.java`:
- Around line 39-42: Update ExploreServerExecutionService.preDestroy to await
graceful executor termination after calling shutdown, using TimeUnit and an
appropriate timeout so in-flight deletion tasks can complete before shutdown
finishes. Preserve the existing shutdown behavior and handle interruption
according to the service’s established conventions.
In `@src/main/java/org/gridsuite/explore/server/services/ExploreService.java`:
- Around line 210-231: Update deleteElementsFromDirectory so each post-loop
cleanup call—directoryService.deleteElementsFromDirectory for deletedIds and
directoryService.updateElementsStatus for failedIds—is independently guarded
against exceptions, ensuring one cleanup failure does not prevent the other and
both groups receive their intended cleanup or status restoration.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/explore/server/ExploreTest.java`:
- Around line 180-181: Scope the WireMockServer lifecycle to
testDeleteElementSpawnsDeletionThread instead of the class-wide setup. Remove
its `@BeforeEach` initialization and `@AfterEach` shutdown, instantiate and start it
within that test, and stop it when the test completes while preserving the
existing wireMockServer stubbing and usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2f211b8-f49a-4c95-b5f8-d8b3a8c46b89
📒 Files selected for processing (6)
src/main/java/org/gridsuite/explore/server/dto/DirectoryElementStatus.javasrc/main/java/org/gridsuite/explore/server/services/DirectoryService.javasrc/main/java/org/gridsuite/explore/server/services/ExploreServerExecutionService.javasrc/main/java/org/gridsuite/explore/server/services/ExploreService.javasrc/test/java/org/gridsuite/explore/server/ExploreTest.javasrc/test/java/org/gridsuite/explore/server/SpreadsheetConfigCollectionTest.java
| @PreDestroy | ||
| private void preDestroy() { | ||
| executorService.shutdown(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Executor shutdown doesn't wait for in-flight deletion tasks.
shutdown() alone stops new task submission but returns immediately; it doesn't block until running tasks finish. On app shutdown/redeploy, in-flight async deletion tasks (multi-step directory-server calls) can be cut off mid-flight, leaving elements stuck in DELETING status.
🔧 Proposed fix to wait for graceful termination
`@PreDestroy`
private void preDestroy() {
- executorService.shutdown();
+ executorService.shutdown();
+ try {
+ if (!executorService.awaitTermination(30, TimeUnit.SECONDS)) {
+ executorService.shutdownNow();
+ }
+ } catch (InterruptedException e) {
+ executorService.shutdownNow();
+ Thread.currentThread().interrupt();
+ }
}Requires adding import java.util.concurrent.TimeUnit;.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/gridsuite/explore/server/services/ExploreServerExecutionService.java`
around lines 39 - 42, Update ExploreServerExecutionService.preDestroy to await
graceful executor termination after calling shutdown, using TimeUnit and an
appropriate timeout so in-flight deletion tasks can complete before shutdown
finishes. Preserve the existing shutdown behavior and handle interruption
according to the service’s established conventions.
| public CompletableFuture<Void> deleteElementsFromDirectory(List<UUID> uuids, UUID parentDirectoryUuid, String userId) { | ||
| return exploreServerExecutionService.runAsync(() -> { | ||
| directoryService.updateElementsStatus(uuids, DirectoryElementStatus.DELETING, userId); | ||
| List<UUID> deletedIds = new ArrayList<>(); | ||
| List<UUID> failedIds = new ArrayList<>(); | ||
| for (UUID id : uuids) { | ||
| try { | ||
| directoryService.deleteElement(id, userId); | ||
| deletedIds.add(id); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Failed to delete element {}", id, e); | ||
| failedIds.add(id); | ||
| } | ||
| } | ||
| if (!deletedIds.isEmpty()) { | ||
| directoryService.deleteElementsFromDirectory(deletedIds, parentDirectoryUuid, userId); | ||
| } | ||
| if (!failedIds.isEmpty()) { | ||
| directoryService.updateElementsStatus(failedIds, DirectoryElementStatus.ACTIVE, userId); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Elements can be left permanently stuck in DELETING if the post-loop cleanup calls fail.
The per-element try/catch (lines 216-223) only guards against individual deletion failures. If directoryService.deleteElementsFromDirectory(deletedIds, ...) (line 225) or the subsequent updateElementsStatus(failedIds, ACTIVE, ...) (line 228) itself throws, the exception escapes uncaught: elements in deletedIds were already deleted from their owning services but never removed from the directory listing (stuck DELETING), and failedIds never get reverted to ACTIVE. This is a related-but-distinct gap from the per-element failure handling already discussed in past review (which added the deletedIds/failedIds tracking but didn't protect these two follow-up calls).
🔧 Proposed fix to guard the cleanup calls
if (!deletedIds.isEmpty()) {
- directoryService.deleteElementsFromDirectory(deletedIds, parentDirectoryUuid, userId);
+ try {
+ directoryService.deleteElementsFromDirectory(deletedIds, parentDirectoryUuid, userId);
+ } catch (Exception e) {
+ LOGGER.error("Failed to remove deleted elements {} from directory", deletedIds, e);
+ }
}
if (!failedIds.isEmpty()) {
- directoryService.updateElementsStatus(failedIds, DirectoryElementStatus.ACTIVE, userId);
+ try {
+ directoryService.updateElementsStatus(failedIds, DirectoryElementStatus.ACTIVE, userId);
+ } catch (Exception e) {
+ LOGGER.error("Failed to restore ACTIVE status for {}", failedIds, e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public CompletableFuture<Void> deleteElementsFromDirectory(List<UUID> uuids, UUID parentDirectoryUuid, String userId) { | |
| return exploreServerExecutionService.runAsync(() -> { | |
| directoryService.updateElementsStatus(uuids, DirectoryElementStatus.DELETING, userId); | |
| List<UUID> deletedIds = new ArrayList<>(); | |
| List<UUID> failedIds = new ArrayList<>(); | |
| for (UUID id : uuids) { | |
| try { | |
| directoryService.deleteElement(id, userId); | |
| deletedIds.add(id); | |
| } catch (Exception e) { | |
| LOGGER.error("Failed to delete element {}", id, e); | |
| failedIds.add(id); | |
| } | |
| } | |
| if (!deletedIds.isEmpty()) { | |
| directoryService.deleteElementsFromDirectory(deletedIds, parentDirectoryUuid, userId); | |
| } | |
| if (!failedIds.isEmpty()) { | |
| directoryService.updateElementsStatus(failedIds, DirectoryElementStatus.ACTIVE, userId); | |
| } | |
| }); | |
| } | |
| public CompletableFuture<Void> deleteElementsFromDirectory(List<UUID> uuids, UUID parentDirectoryUuid, String userId) { | |
| return exploreServerExecutionService.runAsync(() -> { | |
| directoryService.updateElementsStatus(uuids, DirectoryElementStatus.DELETING, userId); | |
| List<UUID> deletedIds = new ArrayList<>(); | |
| List<UUID> failedIds = new ArrayList<>(); | |
| for (UUID id : uuids) { | |
| try { | |
| directoryService.deleteElement(id, userId); | |
| deletedIds.add(id); | |
| } catch (Exception e) { | |
| LOGGER.error("Failed to delete element {}", id, e); | |
| failedIds.add(id); | |
| } | |
| } | |
| if (!deletedIds.isEmpty()) { | |
| try { | |
| directoryService.deleteElementsFromDirectory(deletedIds, parentDirectoryUuid, userId); | |
| } catch (Exception e) { | |
| LOGGER.error("Failed to remove deleted elements {} from directory", deletedIds, e); | |
| } | |
| } | |
| if (!failedIds.isEmpty()) { | |
| try { | |
| directoryService.updateElementsStatus(failedIds, DirectoryElementStatus.ACTIVE, userId); | |
| } catch (Exception e) { | |
| LOGGER.error("Failed to restore ACTIVE status for {}", failedIds, e); | |
| } | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/gridsuite/explore/server/services/ExploreService.java`
around lines 210 - 231, Update deleteElementsFromDirectory so each post-loop
cleanup call—directoryService.deleteElementsFromDirectory for deletedIds and
directoryService.updateElementsStatus for failedIds—is independently guarded
against exceptions, ensuring one cleanup failure does not prevent the other and
both groups receive their intended cleanup or status restoration.
|
2 participants
PR Summary