idantepper commented on code in PR #4728:
URL: https://github.com/apache/solr/pull/4728#discussion_r3775313502
##########
solr/core/src/java/org/apache/solr/handler/SnapShooter.java:
##########
@@ -228,7 +234,42 @@ public static IndexCommit
getAndSaveNamedIndexCommit(SolrCore solrCore, String c
+ solrCore.getName());
}
+ /**
+ * The status of a snapshot that has been requested but has not finished
yet. A null {@link
+ * #snapshotName} is omitted rather than reported, matching how {@link
CoreSnapshotResponse}
+ * reports the same snapshot once it has completed.
+ */
+ private NamedList<Object> inProgressDetails(String startTime, String status)
{
+ NamedList<Object> details = new SimpleOrderedMap<>();
+ details.add("startTime", startTime);
+ details.add("status", status);
+ if (snapshotName != null) {
+ details.add("snapshotName", snapshotName);
+ }
+ details.add("directoryName", directoryName);
+ return details;
+ }
+
+ /**
+ * The status of a snapshot whose files are being copied. Only reported once
the index commit has
+ * been resolved, since until then there is no file list to count.
+ *
+ * @param fileCount the total number of files this snapshot will copy
+ * @param finishedFileCount how many of them have been copied so far
+ */
+ private NamedList<Object> runningDetails(String startTime, int fileCount,
int finishedFileCount) {
+ NamedList<Object> details = inProgressDetails(startTime, RUNNING_STATUS);
+ details.add("fileCount", fileCount);
+ details.add("finishedFileCount", finishedFileCount);
+ return details;
+ }
+
public void createSnapAsync(final int numberToKeep, Consumer<NamedList<?>>
result) {
+ this.progressListener = result;
+ // Report before the thread starts, otherwise the previously reported
status (possibly a
+ // "success" from an earlier snapshot) stays visible until the index
commit has been resolved.
Review Comment:
Yes, exactly that. `snapShootDetails` on `ReplicationHandler` is a plain
volatile field that is never cleared -- `getReplicationDetails` publishes it
under `backup` whenever it is non-null. It gets overwritten by the next backup,
or by a snapshot deletion, and it is only back to `null` after a core reload or
node restart, at which point the `backup` key 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 `success` survived 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 completed `success` payload. I have pushed a
ref-guide commit that adds the two in-progress statuses and states this
retention behaviour explicitly, so a reader knows a `success` may 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 is `exception`.
##########
solr/core/src/test/org/apache/solr/handler/TestSnapshotCoreBackup.java:
##########
@@ -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 {
Review Comment:
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:
- **`TrackingBackupRepository`**
(`solr/test-framework/src/java/org/apache/solr/core/TrackingBackupRepository.java`)
is the nearest, and it is in this same feature area: it wraps the repository
and records every `copyIndexFileFrom` / `createOutput` / `createDirectory` into
a synchronized list so a test can assert on what a backup actually did.
`AbstractIncrementalBackupTest` asserts on `copiedFiles()` / `outputsCreated()`
/ `directoriesCreated()`, as do `LocalFSCloudIncrementalBackupTest` and the
S3/GCS/HDFS backup tests. Same seam as here -- the per-file copy inside a
running backup -- and the same shape.
- **`SoftAutoCommitTest.MockEventListener`** registers a `SolrEventListener`
that offers each async commit / `newSearcher` event into a
`LinkedBlockingQueue`, and the test asserts on the ordering of what arrived.
That is the precedent for asserting on an asynchronous *sequence* rather than
polling for a single state.
- **`SnapshotBackupAPITest.TrackingSnapshotBackupAPI`** overrides
`doSnapShoot` and records what the handler passed instead of running a live
backup -- the same seam this test uses. The `Consumer<NamedList<?>>` overload
of `ReplicationHandler.doSnapShoot` is what `ReplicationHandler` itself calls,
so this is not a test-only backdoor.
The alternative would have been `BackupStatusChecker`, which polls
`command=details` over HTTP (`TestRestoreCore`, `TestReplicationHandlerBackup`,
`TestStressThreadBackup`). I did not use it because it is deliberately
terminal-state-only -- it returns `null` for anything that is not `success` --
and its own javadoc says it is "NOT suitable/safe ... because the replication
handler API provides no reliable way to check the results of a specific backup
before the results of another backup may overwrite them internally". Polling it
for an intermediate status would flake: on a test-sized index the copy loop can
finish between two 50ms polls.
Worth recording that the new statuses do not disturb that helper either --
non-`success` statuses still return `null`, and neither the `exception` check
nor the `startsWith("Unable to delete")` check can match `waiting for commit` /
`running`.
Happy to add an HTTP-level assertion on top if you would prefer one, though
it could only assert "not `success` yet" rather than a specific in-progress
status.
Unrelated: the red check is
`TestGracefulJettyShutdown.testSingleShardInFlightRequestsDuringShutDown`
failing on a jetty HTTP/2 `ClosedChannelException` during shutdown -- nothing
to do with this change; `gradle check` is green.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
