-
Notifications
You must be signed in to change notification settings - Fork 852
SOLR-18344: Report in-progress backup status on /replication?command=details #4728
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | ||
| title: /replication?command=details now reports backup details while the backup is | ||
| still running, including file counts, instead of the previous backup's status | ||
| type: fixed # added, changed, fixed, deprecated, removed, dependency_update, security, other | ||
| authors: | ||
| - name: Idan Tepper | ||
| nick: idantepper | ||
| links: | ||
| - name: SOLR-18344 | ||
| url: https://issues.apache.org/jira/browse/SOLR-18344 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,9 @@ | |
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.concurrent.CopyOnWriteArrayList; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.apache.lucene.index.CheckIndex; | ||
| import org.apache.lucene.index.DirectoryReader; | ||
| import org.apache.lucene.index.IndexCommit; | ||
|
|
@@ -29,9 +32,12 @@ | |
| import org.apache.lucene.tests.util.TestUtil; | ||
| import org.apache.solr.SolrTestCaseJ4; | ||
| import org.apache.solr.common.params.CoreAdminParams; | ||
| import org.apache.solr.common.util.NamedList; | ||
| import org.apache.solr.common.util.TimeSource; | ||
| import org.apache.solr.core.CoreContainer; | ||
| import org.apache.solr.handler.admin.CoreAdminHandler; | ||
| import org.apache.solr.response.SolrQueryResponse; | ||
| import org.apache.solr.util.TimeOut; | ||
| import org.junit.After; | ||
| import org.junit.Before; | ||
|
|
||
|
|
@@ -369,6 +375,101 @@ public void testBackupAfterSoftCommit() throws Exception { | |
| admin.close(); | ||
| } | ||
|
|
||
| /** | ||
| * Backups run asynchronously, so the status reported to /replication?command=details must | ||
| * describe a snapshot that is still running -- not stay silent (or keep describing the previously | ||
| * completed snapshot) until it finishes. | ||
| * | ||
| * <p>Rather than racing a live backup by polling "details", this collects every status the | ||
| * handler would have published and asserts on the whole sequence, which is deterministic. | ||
| */ | ||
| public void testBackupReportsProgressWhileRunning() throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this a pattern that we follow elsewhere in Solr for these types of things?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes -- Solr has a family of "record what the code published, then assert on the collected sequence" helpers rather than racing the thing under test. The three closest to this one:
The alternative would have been Worth recording that the new statuses do not disturb that helper either -- non- Happy to add an HTTP-level assertion on top if you would prefer one, though it could only assert "not Unrelated: the red check is |
||
| for (int i = 0; i < 50; i++) { | ||
| assertU(adoc("id", String.valueOf(i))); | ||
| } | ||
| assertU(commit()); | ||
|
|
||
| final Path backupDir = createTempDir(); | ||
| h.getCoreContainer().getAllowPaths().add(backupDir); | ||
|
|
||
| // this is what /replication?command=backup does, minus the http plumbing | ||
| final List<NamedList<?>> reports = new CopyOnWriteArrayList<>(); | ||
| ReplicationHandler.doSnapShoot( | ||
| 0, 0, backupDir.toString(), null, null, "progress_backup", h.getCore(), reports::add); | ||
|
|
||
| final TimeOut timeOut = new TimeOut(60, TimeUnit.SECONDS, TimeSource.NANO_TIME); | ||
| NamedList<?> last = null; | ||
| while (!timeOut.hasTimedOut()) { | ||
| if (!reports.isEmpty()) { | ||
| last = reports.get(reports.size() - 1); | ||
| assertNull("Backup failed: " + last, last.get("exception")); | ||
| if ("success".equals(last.get("status"))) { | ||
| break; | ||
| } | ||
| } | ||
| timeOut.sleep(20); | ||
| } | ||
| assertNotNull("No backup status was ever reported", last); | ||
| assertEquals( | ||
| "Backup did not succeed before the TimeOut elapsed: " + last, | ||
| "success", | ||
| last.get("status")); | ||
| // the backup is over, so no further reports can arrive and 'reports' is now stable | ||
| final int totalFileCount = ((Number) last.get("fileCount")).intValue(); | ||
| assertTrue( | ||
| "Test needs a backup of more than one file, got " + totalFileCount, 1 < totalFileCount); | ||
|
|
||
| // the very first status is published before the index commit is resolved, so it names no files | ||
| final NamedList<?> waiting = reports.get(0); | ||
| assertEquals( | ||
| "backup should first report itself as waiting: " + waiting, | ||
| SnapShooter.WAITING_FOR_COMMIT_STATUS, | ||
| waiting.get("status")); | ||
| assertNull("no file list is known yet: " + waiting, waiting.get("fileCount")); | ||
| assertNull("no file list is known yet: " + waiting, waiting.get("finishedFileCount")); | ||
|
|
||
| final List<NamedList<?>> running = reports.subList(1, reports.size() - 1); | ||
| assertFalse("Backup was never reported as running", running.isEmpty()); | ||
|
|
||
| int previousFinished = -1; | ||
| for (NamedList<?> report : running) { | ||
| assertEquals( | ||
| "not reported as running: " + report, SnapShooter.RUNNING_STATUS, report.get("status")); | ||
|
|
||
| final int finished = ((Number) report.get("finishedFileCount")).intValue(); | ||
| assertTrue( | ||
| "finishedFileCount went backwards: " + previousFinished + " -> " + finished, | ||
| previousFinished <= finished); | ||
| previousFinished = finished; | ||
| } | ||
|
|
||
| // every in-progress status must identify the backup it is about | ||
| for (NamedList<?> report : reports.subList(0, reports.size() - 1)) { | ||
| assertNotNull("in-progress report has no startTime: " + report, report.get("startTime")); | ||
| assertEquals( | ||
| "in-progress report names the wrong snapshot: " + report, | ||
| "progress_backup", | ||
| report.get("snapshotName")); | ||
| assertEquals( | ||
| "in-progress report has the wrong directoryName: " + report, | ||
| "snapshot.progress_backup", | ||
| report.get("directoryName")); | ||
| } | ||
|
|
||
| // by the last report before completion, every file must be accounted for | ||
| final NamedList<?> lastRunning = running.get(running.size() - 1); | ||
| assertEquals( | ||
| "last running report disagrees with the completed backup: " + lastRunning, | ||
| totalFileCount, | ||
| ((Number) lastRunning.get("fileCount")).intValue()); | ||
| assertEquals( | ||
| "last running report did not finish every file: " + lastRunning, | ||
| totalFileCount, | ||
| ((Number) lastRunning.get("finishedFileCount")).intValue()); | ||
|
|
||
| simpleBackupCheck(backupDir.resolve("snapshot.progress_backup"), 50); | ||
| } | ||
|
|
||
| /** | ||
| * A simple sanity check that asserts the current weird behavior of | ||
| * DirectoryReader.openIfChanged() and demos how 'softCommit' can cause the IndexReader in use by | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is the idea that a success is just going to hangaround for forever, or until the next snapshot is begun? I guess that is how it works...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly that.
snapShootDetailsonReplicationHandleris a plain volatile field that is never cleared --getReplicationDetailspublishes it underbackupwhenever it is non-null. It gets overwritten by the next backup, or by a snapshot deletion, and it is only back tonullafter a core reload or node restart, at which point thebackupkey is simply absent again.That lifetime is pre-existing and this PR does not change it. What it changes is when the first overwrite lands: it used to be at the end of the next backup, so a stale
successsurvived the entire run of the backup that was meant to replace it. Now it is replaced the moment the next backup is requested, which is the part that fixes the stale read.You prompted me to go look at the docs, and my checklist claim was wrong:
backup-restore.adoc-- "Backup Status" does document the shape of this response, it just only showed the completedsuccesspayload. I have pushed a ref-guide commit that adds the two in-progress statuses and states this retention behaviour explicitly, so a reader knows asuccessmay describe an earlier backup rather than one just requested.Drive-by in the same section, shout if you would rather it went separately: it said a failure reports
snapShootException, which appears nowhere in the codebase -- the key isexception.