Recreate search index if its format is outdated #802
No reviewers
Labels
No labels
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Plume/Plume#802
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "KitaitiMakoto/Plume:invalid-index"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hi,
I fixed the problem #788.
This patch detects outdated search index and recreates it.
some questions 🙋🏻♀️
@ -101,0 +102,4 @@
UnmanagedSearcher::open(&CONFIG.search_index, &CONFIG.search_tokenizers);
if let Err(Error::Search(SearcherError::InvalidIndexDataError)) = open_searcher {
UnmanagedSearcher::create(&CONFIG.search_index, &CONFIG.search_tokenizers)
.expect("main: recreating search index error. Try backing up search index, removing it and running `plm search init`");
how unsafe (or unsavoury?) would it be to do the thing we suggest here, instead of suggesting it?
rename the index file to append the date-time, and create a new (empty?) index?
is the functionality for (re)creating an index exclusively in
plm
?Renaming current search index as a backup and creating new index is good. There's a point to discuss: how and who remove that backup?
Administrator might want to restore the index several days later for some purpose. If Plume removes backup directory after it started running successfully, they cannot achieve their purpose. So, remaining backup directory and recomment admin to remove it at any point in time.
On the other hand, most of admins will forget remove backup directories even if Plume show a message recommending removing later. If they remove it themselves, they remember the existence of backup directory and can add removing it to their to-do list. This is why Plume shows message and exists in my patch.
Which is better do you think?
why would we need to keep our restore an index which Plume cannot load?
what could the admin do, that we can't? (why aren't we doing that?)
Admin can run Plume v0.4.0 with older index.
But your point is reasonable. It's the rare case what I imagine. I will rewrite my patch to temporatrilly backup older index and remove it after successfully recreating new index. Thank you for the advice.
I've rewritten. Can you check it out, please?
some comments
@ -101,0 +119,4 @@
backup_path.display()
);
}
Ok(searcher)
who is this return for?
I've removed it!
@ -101,0 +123,4 @@
},
)
})
.expect("main: error on recreating search index in new index format");
any advice on what to do here if this happens?
I added message to take action.
@ -101,1 +126,4 @@
.expect("main: error on recreating search index in new index format");
open_searcher = UnmanagedSearcher::open(&CONFIG.search_index, &CONFIG.search_tokenizers);
}
#[allow(clippy::match_wild_err_arm)]
is this clippy still necessary?
I think so. There are many types of error in Plume and they doesn't matter here. But I'm not good at Rust. Is there better way to the errors?
@ -101,2 +128,3 @@
}
#[allow(clippy::match_wild_err_arm)]
let searcher = match UnmanagedSearcher::open(&CONFIG.search_index, &CONFIG.search_tokenizers) {
let searcher = match open_searcher {
maybe we should put the above
if let
into thismatch
as well…No, what
if let
block does is creating search index. It need to be done before opening search index.@ -101,0 +109,4 @@
let backup_path = format!("{}.{}", ¤t_path.display(), Utc::now().timestamp());
let backup_path = Path::new(&backup_path);
fs::rename(current_path, backup_path)
.expect("main: error on backing up search index directory for recreating");
is this a user-facing error message? should it go thru translation?
This is a message for administrators. I think it's not needed to translate as such other error messages in this file.
administrators are users too.
anyway, let's ear-mark this for a next round
I filed an issue: #803
@ -101,0 +114,4 @@
if fs::remove_dir_all(backup_path).is_err() {
eprintln!(
"error on removing backup directory: {}. it remains",
backup_path.display()
this is a user-facing error message, so it should go thru translation
The same to the above comment.
@ -101,0 +118,4 @@
);
}
} else {
panic!("main: error on recreating search index in new index format. remove search index and run `plm search init` manually");
this is a user-facing error message, so it should go thru translation
Ditto.
👍
Thanks for the merge.