Recreate search index if its format is outdated #802

KitaitiMakoto · 2020-07-18T00:51:26Z

KitaitiMakoto commented

2020-07-18 00:51:26 +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

Cause IndexInvalidDataError when search index is invalid c5d03d300b

Recreate search index if its format is outdated 50753b3a65

igalic reviewed 2020-07-18 09:53:55 +00:00

igalic left a comment

some questions 🙋🏻‍♀️

src/main.rs

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

igalic commented

2020-07-18 09:53:23 +00:00

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

KitaitiMakoto commented

2020-07-18 10:28:16 +00:00

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?

igalic commented

2020-07-18 10:36:56 +00:00

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

KitaitiMakoto commented

2020-07-18 10:39:53 +00:00

Admin can run Plume v0.4.0 with older index.

KitaitiMakoto commented

2020-07-18 10:42:37 +00:00

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.

KitaitiMakoto commented

2020-07-18 14:33:11 +00:00

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

Re-initialize search index when recreating is failed 5fc827c1c9

KitaitiMakoto added 1 commit 2020-07-18 16:24:12 +00:00

Run cargo fmt 484659fde2

igalic reviewed 2020-07-18 19:10:03 +00:00

igalic left a comment

some comments

src/main.rs Outdated

					
				@ -101,0 +119,4 @@

				                                backup_path.display()

				                            );

				                        }

				                        Ok(searcher)

igalic commented

2020-07-18 19:08:33 +00:00

who is this return for?

KitaitiMakoto commented

2020-07-19 02:56:49 +00:00

I've removed it!

src/main.rs Outdated

					
				@ -101,0 +123,4 @@

				                    },

				                )

				            })

				            .expect("main: error on recreating search index in new index format");

igalic commented

2020-07-18 18:56:56 +00:00

any advice on what to do here if this happens?

KitaitiMakoto commented

2020-07-19 02:57:13 +00:00

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

igalic commented

2020-07-18 18:56:16 +00:00

is this clippy still necessary?

KitaitiMakoto commented

2020-07-19 02:53:21 +00:00

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 {

igalic commented

2020-07-18 19:09:53 +00:00

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…

KitaitiMakoto commented

2020-07-19 02:55:48 +00:00

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

Add action user can take to error message 5cca66b346

Don't care about needless return value of closures 5e30bede40

igalic reviewed 2020-07-19 06:41:50 +00:00

src/main.rs

					
				@ -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");

igalic commented

2020-07-19 06:41:11 +00:00

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

KitaitiMakoto commented

2020-07-19 07:43:02 +00:00

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

igalic commented

2020-07-19 08:01:52 +00:00

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

KitaitiMakoto commented

2020-07-19 08:35:07 +00:00

I filed an issue: #803

I filed an issue: https://git.joinplu.me/Plume/Plume/issues/803

igalic marked this conversation as resolved

src/main.rs

					
				@ -101,0 +114,4 @@

				                if fs::remove_dir_all(backup_path).is_err() {

				                    eprintln!(

				                        "error on removing backup directory: {}. it remains",

				                        backup_path.display()

igalic commented

2020-07-19 06:38:26 +00:00

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

KitaitiMakoto commented

2020-07-19 07:43:31 +00:00

The same to the above comment.

igalic marked this conversation as resolved

src/main.rs

					
				@ -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");