Recreate search index if its format is outdated #802

Merged

kiwii merged 6 commits from KitaitiMakoto/Plume:invalid-index into main

KitaitiMakoto commented

Owner

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

c5d03d300b Cause IndexInvalidDataError when search index is invalid

50753b3a65 Recreate search index if its format is outdated

igalic reviewed

igalic left a comment

Owner

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

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

KitaitiMakoto commented

Poster

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?

igalic commented

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

KitaitiMakoto commented

Poster

Owner

Admin can run Plume v0.4.0 with older index.

KitaitiMakoto commented

Poster

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.

KitaitiMakoto commented

Poster

Owner

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

igalic marked this conversation as resolved

KitaitiMakoto added 1 commit

5fc827c1c9 Re-initialize search index when recreating is failed

KitaitiMakoto added 1 commit

484659fde2 Run cargo fmt

igalic reviewed

igalic left a comment

Owner

some comments

src/main.rs Outdated

													
				@ -101,0 +119,4 @@

				                                backup_path.display()

				                            );

				                        }

				                        Ok(searcher)

igalic commented

Owner

who is this return for?

KitaitiMakoto commented

Poster

Owner

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

Owner

any advice on what to do here if this happens?

KitaitiMakoto commented

Poster

Owner

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

Owner

is this clippy still necessary?

KitaitiMakoto commented

Poster

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?

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

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…

KitaitiMakoto commented

Poster

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

5cca66b346 Add action user can take to error message

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

igalic reviewed

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

Owner

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

KitaitiMakoto commented

Poster

Owner

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

igalic commented

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

KitaitiMakoto commented

Poster

Owner

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

Owner

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

KitaitiMakoto commented

Poster

Owner

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

igalic commented

Owner

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

KitaitiMakoto commented

Poster

Owner

Ditto.

igalic marked this conversation as resolved

igalic approved these changes

igalic left a comment

Owner

👍

KitaitiMakoto referenced this pull request

I18n for messages in terminal #803

igalic referenced this pull request

all User facing error messages should be translated #804

kiwii referenced this issue from a commit

Merge pull request 'Recreate search index if its format is outdated' (#802) from KitaitiMakoto/Plume:invalid-index into main

kiwii merged commit 6de9a1f1c8 into main 4 years ago

KitaitiMakoto commented

Poster

Owner

Thanks for the merge.

KitaitiMakoto deleted branch invalid-index

Reviewers

igalic approved these changes

The pull request has been merged as 6de9a1f1c8.

No reviewers

No Milestone

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.

Loading…

There is no content yet.

Labels Milestones

Recreate search index if its format is outdated #802

Reviewers