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

Refactored expired search and session deletion to delete records in s… #739

Merged
merged 4 commits into from Jul 7, 2016

Conversation

EreMaijala
Copy link
Contributor

…mall batches to avoid locking the tables for too long or creating a massive transaction in case the table has accumulated records for a longer time.

I also added timestamps to the messages the expiration process prints out so that e.g. when there's a gap in the ID range, it doesn't look like nothing's happening. And having the timestamp is useful too when piping the output to a log file.

…mall batches to avoid locking the tables for too long or creating a massive transaction in case the table has accumulated records for a longer time.
@demiankatz
Copy link
Member

Thanks for this. I've just taken a very quick glance at this and will probably not have time to do detailed work on it until after vacation... but a few first thoughts to perhaps keep things moving along:

1.) The delete logic is present not just in the console utilities but also in the web-based admin panel (see this code). Your refactoring to remove getExpiredQuery is going to break the generic design the admin panel uses for expiring various types of things. We either need to keep the old method for backward compatibility or else revise the admin panel code.

2.) It might make sense to add a switch to the command line to set the sleep time for the loop; some people may want this, others may find it problematic. Chunk size might also be a useful configurable setting.

3.) Minor nitpick, but I'd rename the message() method you've added to the console controller to something like datedMessage() to clarify its purpose.

…ers for specifying batch size and sleep time.
@EreMaijala
Copy link
Contributor Author

1.) Oops. I reverted the deletion since I'm not sure the batch stuff works well with the admin panel (which I don't have experience with).

2.) True, done. I modified the help message to accommodate this. Do you think this makes sense? I also simplified the help param check since GetOpt is supposed to always return the non-alias version of the parameter.

3.) Done.

@demiankatz
Copy link
Member

Looks good. Thanks for the revisions! I just made one more small revision to avoid sleeping after the last batch; no point in waiting around once the job is done.

@demiankatz demiankatz merged commit 4565b92 into vufind-org:master Jul 7, 2016
@EreMaijala EreMaijala deleted the expire-batch branch August 23, 2016 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants