Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speed up migrationPage #4417

Merged
merged 2 commits into from
May 20, 2021

Conversation

Kathrin-Huber
Copy link
Contributor

fixes #4384

Copy link
Collaborator

@matthias-ronge matthias-ronge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes method processOwnsYearXML(Process process) in FileService dead code and you could delete it. Alternative suggestion: You combine the two methods processOwnsYearXML(Process process) and processOwnsYearXML(Process process, boolean forIndexingAll) in FileService (line 589−603) to this one method:

public boolean processOwnsYearXML(Process process) throws IOException {
    URI yearFile = createYearFile(getMetadataFilePath(process, false, true));
    return fileExist(yearFile);
}

That should have the same effect.

@Kathrin-Huber
Copy link
Contributor Author

Thats a pretty good idea! That way I also found that processOwnsAnchorXML was dead.

Copy link
Collaborator

@henning-gerhardt henning-gerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your changes did not solve the issues mentioned in #4384 as your changes are inside the migration itself and not on displaying the newspaper batches. So you may rename the pull request title and remove the "fixes #4384".

@henning-gerhardt
Copy link
Collaborator

I must partial revert my last comment as I got confused by the changes which including some refactoring. On a new migrated database the values for processBaseUri did not get updated any more and this is a big improvement for migration speed. But this pull request (or at least the current version of it) did not solve the slow page displaying at all as the check for the year.xml files are still executed and this takes still a lot of time (even less then storing the changed value in processBaseUri to the database and maybe index).

So far as I know the database and its entries there is a type field in batch table which is holding the information about what kind of batch is stored. Is this not enough to decide the displayed batches on the newspaper migration page?

@matthias-ronge
Copy link
Collaborator

Slow file system access is a different issue (#3300) which also needs to be taken looked after, but I think this pull request fixes the database updates, and this is what talks are about in #4384

Using the batch.type field should be the best solution to find newspaper batches, I agree to this.

@Kathrin-Huber Kathrin-Huber merged commit 5b3a60e into kitodo:master May 20, 2021
@Kathrin-Huber Kathrin-Huber deleted the speed_up_migrationPage branch May 20, 2021 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not useable newspaper migration site
3 participants