epugh commented on code in PR #4808:
URL: https://github.com/apache/solr/pull/4808#discussion_r3897095965
##########
solr/core/src/java/org/apache/solr/cli/SnapshotExportTool.java:
##########
@@ -115,7 +115,6 @@ public void exportSnapshot(
try {
CollectionAdminRequest.Backup backup =
new CollectionAdminRequest.Backup(collectionName, snapshotName);
- backup.setCommitName(snapshotName);
Review Comment:
Reviewing this against the removal of the full-snapshot backup path:
`SnapshotExportTool` (`bin/solr snapshot-export`) has a leftover issue with
`--snapshot-name`.
Before this PR, `--snapshot-name` selected a previously-created named
snapshot to export (via `backup.setCommitName(snapshotName)`, removed here).
After this PR, that selection mechanism is gone, but `--snapshot-name` is still
a **required** CLI argument — its value now just gets passed as the *new
backup's name* (`new CollectionAdminRequest.Backup(collectionName,
snapshotName)`), with no indication to the user that it no longer does what its
name/help text implies.
Concretely: a user runs `bin/solr snapshot-export -c coll --snapshot-name
snap1 --dest-dir /tmp/x` expecting to export the state of `coll` as of `snap1`.
Instead they silently get a backup of `coll`'s *current* live state. That's a
correctness footgun, not just a docs nit — worth fixing before this merges (or
as an immediate follow-up).
Suggested fix: make `--snapshot-name` non-required, reject it explicitly
with a clear error if passed (e.g. `"--snapshot-name is no longer supported...
it always backs up the collection's current state"`), and derive the backup's
internal name from the collection name + timestamp instead of from user input.
I've got a working patch for this (compiles clean, spotless-formatted) if
useful — happy to share the diff or open it as a follow-up PR against this
branch.
--
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]
