Recreate search index if its format is outdated #802

Merged
kiwii merged 6 commits from KitaitiMakoto/Plume:invalid-index into main 2020-07-19 12:04:44 +00:00

Hi,

I fixed the problem #788.

This patch detects outdated search index and recreates it.

Hi, I fixed the problem #788. This patch detects outdated search index and recreates it.
KitaitiMakoto added 2 commits 2020-07-18 00:51:26 +00:00
igalic reviewed 2020-07-18 09:53:55 +00:00
igalic left a comment
Owner

some questions 🙋🏻‍♀️

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`");
Owner

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?

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`?
Author
Owner

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?

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?
Owner

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?)

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?)
Author
Owner

Admin can run Plume v0.4.0 with older index.

Admin can run Plume v0.4.0 with older index.
Author
Owner

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.

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.
Author
Owner

I've rewritten. Can you check it out, please?

I've rewritten. Can you check it out, please?
igalic marked this conversation as resolved
KitaitiMakoto added 1 commit 2020-07-18 14:32:32 +00:00
KitaitiMakoto added 1 commit 2020-07-18 16:24:12 +00:00
igalic reviewed 2020-07-18 19:10:03 +00:00
igalic left a comment
Owner

some comments

some comments
src/main.rs Outdated
@ -101,0 +119,4 @@
backup_path.display()
);
}
Ok(searcher)
Owner

who is this return for?

who is this return for?
Author
Owner

I've removed it!

I've removed it!
src/main.rs Outdated
@ -101,0 +123,4 @@
},
)
})
.expect("main: error on recreating search index in new index format");
Owner

any advice on what to do here if this happens?

any advice on what to do here if this happens?
Author
Owner

I added message to take action.

I added message to take action.
src/main.rs Outdated
@ -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)]
Owner

is this clippy still necessary?

is this clippy still necessary?
Author
Owner

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?

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?
src/main.rs Outdated
@ -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 {
Owner

maybe we should put the above if let into this match as well…

maybe we should put the above `if let` into this `match` as well…
Author
Owner

No, what if let block does is creating search index. It need to be done before opening search index.

No, what `if let` block does is *creating* search index. It need to be done before *opening* search index.
KitaitiMakoto added 2 commits 2020-07-19 02:56:21 +00:00
igalic reviewed 2020-07-19 06:41:50 +00:00
@ -101,0 +109,4 @@
let backup_path = format!("{}.{}", &current_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");
Owner

is this a user-facing error message? should it go thru translation?

is this a user-facing error message? should it go thru translation?
Author
Owner

This is a message for administrators. I think it's not needed to translate as such other error messages in this file.

This is a message for administrators. I think it's not needed to translate as such other error messages in this file.
Owner

administrators are users too.
anyway, let's ear-mark this for a next round

administrators are users too. anyway, let's ear-mark this for a next round
Author
Owner

I filed an issue: #803

I filed an issue: https://git.joinplu.me/Plume/Plume/issues/803
igalic marked this conversation as resolved
@ -101,0 +114,4 @@
if fs::remove_dir_all(backup_path).is_err() {
eprintln!(
"error on removing backup directory: {}. it remains",
backup_path.display()
Owner

this is a user-facing error message, so it should go thru translation

this is a user-facing error message, so it should go thru translation
Author
Owner

The same to the above comment.

The same to the above comment.
igalic marked this conversation as resolved
@ -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");
Owner

this is a user-facing error message, so it should go thru translation

this is a user-facing error message, so it should go thru translation
Author
Owner

Ditto.

Ditto.
igalic marked this conversation as resolved
igalic approved these changes 2020-07-19 08:03:18 +00:00
igalic left a comment
Owner

👍

👍
kiwii merged commit 6de9a1f1c8 into main 2020-07-19 12:04:43 +00:00
Author
Owner

Thanks for the merge.

Thanks for the merge.
KitaitiMakoto deleted branch invalid-index 2020-07-19 21:35:36 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#802
No description provided.