You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@polaris.apache.org by "ayushtkn (via GitHub)" <gi...@apache.org> on 2026/10/14 02:38:57 UTC

[PR] NoSQL maintenance: Fix maintenance-run exiting with success after a failed run [polaris]

ayushtkn opened a new pull request, #5786:
URL: https://github.com/apache/polaris/pull/5786

   The `nosql` maintenance-run admin command exits with status `0` even when the maintenance run failed, so a cron job or orchestrator driving it cannot tell a failed run from a successful one and will happily carry on as if the database had been cleaned. 
   A failed run is not signalled by an exception: the service catches everything and records the outcome on the returned run information, whose success flag even defaults to `false` so that a run interrupted before it completes is reported as unsuccessful too. The command ignores that flag and returns `0` from the one path that can reach it, while already returning a dedicated maintenance error code on the neighbouring path for a run it refuses to start — so the exit code is derived from the run information here, using that same code. The run information printed to stdout is unaffected; it already carries the status message and the failure detail, and only the exit status was wrong.
   
   ## Checklist
   - [ ] 🛡️ Don't disclose security issues! (contact security@apache.org)
   - [ ] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
   - [ ] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [ ] 💡 Added comments for complex logic
   - [ ] 🧾 Updated `CHANGELOG.md` (if needed)
   - [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] NoSQL maintenance: Fix maintenance-run exiting with success after a failed run [polaris]

Posted by "snazy (via GitHub)" <gi...@apache.org>.

snazy commented on PR #5786:
URL: https://github.com/apache/polaris/pull/5786#issuecomment-6189542179

   Thanks @ayushtkn !


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] NoSQL maintenance: Fix maintenance-run exiting with success after a failed run [polaris]

Posted by "snazy (via GitHub)" <gi...@apache.org>.

snazy merged PR #5786:
URL: https://github.com/apache/polaris/pull/5786


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] NoSQL maintenance: Fix maintenance-run exiting with success after a failed run [polaris]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #5786:
URL: https://github.com/apache/polaris/pull/5786#discussion_r4266287341


##########
runtime/admin/src/test/java/org/apache/polaris/admintool/nosql/TestNoSqlMaintenanceRunCommand.java:
##########
@@ -69,6 +69,34 @@ public void passesSupersedeRunIdToMaintenanceService() throws Exception {
     assertThat(stderr.toString()).isEmpty();
   }
 
+  @Test
+  public void reportsMaintenanceErrorWhenRunDidNotSucceed() throws Exception {
+    var service = new RecordingMaintenanceService();
+    service.runInformation =
+        MaintenanceRunInformation.builder()
+            .started(Instant.parse("2026-05-11T10:15:30Z"))
+            .finished(Instant.parse("2026-05-11T10:15:31Z"))
+            .success(false)
+            .statusMessage("Maintenance run did not finish successfully.")
+            .build();
+
+    var command = new NoSqlMaintenanceRunCommand();
+    inject(command, "maintenanceService", service);
+    inject(command, "maintenanceConfig", MaintenanceConfig.builder().build());
+    inject(command, "backend", backend("MongoDb"));
+
+    var stdout = new StringWriter();
+    var stderr = new StringWriter();
+    var exitCode =
+        new CommandLine(command)
+            .setOut(new PrintWriter(stdout, true))
+            .setErr(new PrintWriter(stderr, true))
+            .execute();
+
+    assertThat(exitCode).isEqualTo(BaseCommand.EXIT_CODE_MAINTENANCE_ERROR);
+    assertThat(stdout.toString()).contains("Maintenance run did not finish successfully.");

Review Comment:
   `stderr` is captured but never asserted (or otherwise used), so the test would still pass even if the command unexpectedly prints errors to stderr. Consider adding `assertThat(stderr.toString()).isEmpty()` (or asserting the expected stderr content), or remove the stderr wiring if it’s intentionally ignored.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org