|
Description
Jonathan Druart
2015-01-21 14:34:01 UTC
Created attachment 35444 [details] [review] Bug 13606: Remove previous job data from the session Bug 11395 uses the session to store the report sent to the interface (update on biblio XX OK, etc.). If the session is stored in mysql, the max size of the sessions.a_session will be reached easily (TEXT field). To reproduce: 0/ Set SessionStorage to 'mysql' 1/ Create a file with 500 biblionumbers: mysql -e "select biblionumber from biblio limit 500;" | tail -n 500 > /tmp/biblionumbers.txt 2/ Define a marc modification template (something like "delete field 99$9" is nice) 3/ Load the /tmp/biblionumbers.txt in the batch biblio modification tool 4/ Repeat 3 You will get: Syck parser (line 1534, column 6): syntax error at /usr/lib/i386-linux-gnu/perl5/5.20/YAML/Syck.pm line 75. None Koha page is reachable. It comes from get_template_and_user > checkauth > get_session > l.1595 $session = new CGI::Session("driver:MySQL;serializer:yaml;id:md5", $sessionID, {Handle=>$dbh}); 5/ Get your sessionID contained in your CGISESSID cookie Have a look at the value a_session in the sessions table: mysql> select a_session from sessions where id="YOUR_SESSIONID"; You should see that the yaml is not correctly ended. The size of the DB field has been reached and the yaml is truncated. Test plan: 0/ Delete your CGISESSID cookie 1/ Try to reproduce the previous issue 2/ After the second batch, have a look at the sessions table and confirm that only one job_$JOB_ID exist in the yaml Created attachment 35445 [details] [review] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! The second patch doesn't apply: fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Le dépôt n'a pas les blobs nécessaires pour un retour à une fusion à 3 points. Impossible de retourner à une fusion à 3 points. Created attachment 37597 [details] [review] [SIGNED-OFF] Bug 13606: Remove previous job data from the session Bug 11395 uses the session to store the report sent to the interface (update on biblio XX OK, etc.). If the session is stored in mysql, the max size of the sessions.a_session will be reached easily (TEXT field). To reproduce: 0/ Set SessionStorage to 'mysql' 1/ Create a file with 500 biblionumbers: mysql -e "select biblionumber from biblio limit 500;" | tail -n 500 > /tmp/biblionumbers.txt 2/ Define a marc modification template (something like "delete field 99$9" is nice) 3/ Load the /tmp/biblionumbers.txt in the batch biblio modification tool 4/ Repeat 3 You will get: Syck parser (line 1534, column 6): syntax error at /usr/lib/i386-linux-gnu/perl5/5.20/YAML/Syck.pm line 75. None Koha page is reachable. It comes from get_template_and_user > checkauth > get_session > l.1595 $session = new CGI::Session("driver:MySQL;serializer:yaml;id:md5", $sessionID, {Handle=>$dbh}); 5/ Get your sessionID contained in your CGISESSID cookie Have a look at the value a_session in the sessions table: mysql> select a_session from sessions where id="YOUR_SESSIONID"; You should see that the yaml is not correctly ended. The size of the DB field has been reached and the yaml is truncated. Test plan: 0/ Delete your CGISESSID cookie 1/ Try to reproduce the previous issue 2/ After the second batch, have a look at the sessions table and confirm that only one job_$JOB_ID exist in the yaml Signed-off-by: Frederic Demians <f.demians@tamil.fr> I can reproduce de bug. I can see also the useless C4::BackgroundJob session data. I confirm that modifying two consecutive 500 records batches broke entirely the session YAML representation deserialization. And finally I confirm that this patch fixes all those issues. Created attachment 37601 [details] [review] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! I am raising the priority of this one, it should absolutely be present in 3.20. Actually I am wondering what happens if 2 jobs are launched simultaneously :-/ Created attachment 37621 [details] [review] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I confirm that with more than 1000 records the session object is truncated, and then can't be anymore be desirialized, blocking completely Koha. Fixed with this patch. (In reply to Jonathan Druart from comment #7) > Actually I am wondering what happens if 2 jobs are launched simultaneously > :-/ Good question! Above ~1000 records, the processing time is long, and the UI feedback is not that good. So users could be tempted to refresh the page. It may be advisable to limit processing to let say 1000 records. (In reply to Frédéric Demians from comment #9) > (In reply to Jonathan Druart from comment #7) > > Actually I am wondering what happens if 2 jobs are launched simultaneously > > :-/ > > Good question! Above ~1000 records, the processing time is long, and the UI > feedback is not that good. So users could be tempted to refresh the page. It > may be advisable to limit processing to let say 1000 records. I would be tempt to only keep the DB changes, not the first patch. I did not try, but if several jobs are launched (any jobs using the BackgroundJob module), the last one will erase data from previous ones. The first patch should be improved to only delete data from finished jobs. Created attachment 37627 [details] [review] Bug 13606: Remove previous job data from the session (In reply to Jonathan Druart from comment #11) > The first patch should be improved to only delete data from finished jobs. Frederic, I rewrote my first patch. The job entries in the session are now deleted when a new job is created. It looks better like that and we should not get issue with simultaneous jobs. Could you confirm the progress bar is broken, even on the items modification tool? I tried with an old version of the code, and same issue. Not sure if it comes from me or if it's an issue... Why is there no "Started: 2015-04-09 12:58:00" type value in the job session information? You could easily also delete anything that is more than 24 hours old, if there was a way to determine it. Also, perhaps a 'LastTouched: 2015-04-09 15:07:00' type entry would be useful too. If it isn't touched in 30 minutes, is it even running? I successfully (and that's not a good thing) ran two batch modify concurrently, and one progress bar hung, but the other finished. I suspect the process was killed by OS, because the javascript kept pulling the 99% just fine. However, on a positive note, this does make a significant improvement. In the same kind of test ran multiple times, I actually had the error (mentioned in comment #1) triggered behind the scenes and the progress bar seemed to get stuck before the patch. I did not encounter the same problem after. Though, the patch for the paragraph 1 problem would likely solve, as a side-effect the hidden hang in the paragraph 2 problem. If we store the process ID of the child in the job session, then the progress javascript call can check for the existence of that process if the status is not completed. If the process does not exist, then it can change the status to failed OR perhaps attempt to restart with a smaller data set in order to continue. This, of course, is discussion for the creation of another bug report. Created attachment 37633 [details] [review] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I confirm that with more than 1000 records the session object is truncated, and then can't be anymore be desirialized, blocking completely Koha. Fixed with this patch. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> This only pushes off the problem expontentially (64KB vs. 16MB), but this is a good start. Created attachment 37634 [details] [review] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I confirm that with more than 1000 records the session object is truncated, and then can't be anymore be desirialized, blocking completely Koha. Fixed with this patch. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> This only pushes off the problem expontentially (64KB vs. 16MB), but this is a good start. Created attachment 37635 [details] [review] Bug 13606: Remove previous job data from the session Bug 11395 uses the session to store the report sent to the interface (update on biblio XX OK, etc.). If the session is stored in mysql, the max size of the sessions.a_session will be reached easily (TEXT field). To reproduce: 0/ Set SessionStorage to 'mysql' 1/ Create a file with 500 biblionumbers: mysql -e "select biblionumber from biblio limit 500;" | tail -n 500 > /tmp/biblionumbers.txt 2/ Define a marc modification template (something like "delete field 99$9" is nice) 3/ Load the /tmp/biblionumbers.txt in the batch biblio modification tool 4/ Repeat 3 You will get: Syck parser (line 1534, column 6): syntax error at /usr/lib/i386-linux-gnu/perl5/5.20/YAML/Syck.pm line 75. None Koha page is reachable. It comes from get_template_and_user > checkauth > get_session > l.1595 $session = new CGI::Session("driver:MySQL;serializer:yaml;id:md5", $sessionID, {Handle=>$dbh}); 5/ Get your sessionID contained in your CGISESSID cookie Have a look at the value a_session in the sessions table: mysql> select a_session from sessions where id="YOUR_SESSIONID"; You should see that the yaml is not correctly ended. The size of the DB field has been reached and the yaml is truncated. Test plan: 0/ Delete your CGISESSID cookie 1/ Try to reproduce the previous issue 2/ After the second batch, have a look at the sessions table and confirm that only one job_$JOB_ID exist in the yaml Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Confirmed that last completed run is the only thing in the session, under normal circumstances, now. This will go a long way to preventing the 16MB MEDIUMTEXT from filling up. Jonathan, reading your code, I understand better now how it interact with C4::Background module. Your last patch clear the batch modification job data when a new job is created. Wouldn't it be simpler to clear it just after the job is completed in batch_record_modification.pl? I will do a proposal patch for that, hoping I don't miss something important. And, yes, I confirm that progress bar is also broken in batch item modification. It may works that way since a while. Created attachment 37636 [details] [review] Proposal to clear the job session data at the end of the process Created attachment 37637 [details] [review] Bug 13606 Clear job data in session when the job is completed Bug 11395 uses the session to store the report sent to the interface (update on biblio XX OK, etc.). If the session is stored in mysql, the max size of the sessions.a_session will be reached easily (TEXT field). To reproduce: 0/ Set SessionStorage to 'mysql' 1/ Create a file with 500 biblionumbers: mysql -e "select biblionumber from biblio limit 500;" | tail -n 500 > /tmp/biblionumbers.txt 2/ Define a marc modification template (something like "delete field 99$9" is nice) 3/ Load the /tmp/biblionumbers.txt in the batch biblio modification tool 4/ Repeat 3 You will get: Syck parser (line 1534, column 6): syntax error at /usr/lib/i386-linux-gnu/perl5/5.20/YAML/Syck.pm line 75. None Koha page is reachable. It comes from get_template_and_user > checkauth > get_session > l.1595 $session = new CGI::Session("driver:MySQL;serializer:yaml;id:md5", $sessionID, {Handle=>$dbh}); 5/ Get your sessionID contained in your CGISESSID cookie Have a look at the value a_session in the sessions table: mysql> select a_session from sessions where id="YOUR_SESSIONID"; You should see that the yaml is not correctly ended. The size of the DB field has been reached and the yaml is truncated. Test plan: 0/ Delete your CGISESSID cookie 1/ Try to reproduce the previous issue 2/ After the second batch, have a look at the sessions table and confirm that only one job_$JOB_ID exist in the yaml Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> (In reply to Frédéric Demians from comment #20) > Jonathan, reading your code, I understand better now how it interact with > C4::Background module. Your last patch clear the batch modification job data > when a new job is created. Wouldn't it be simpler to clear it just after the > job is completed in batch_record_modification.pl? I will do a proposal patch > for that, hoping I don't miss something important. It works nicely. I have tried to add this line in the finish method, but did not work. The only concern could be in case we need debugging. If something wrong happens it is good to know what kind of batch has been launched. Apart from that my patch was not much better than yours... > And, yes, I confirm that progress bar is also broken in batch item > modification. It may works that way since a while. That's really weird, I'm sure it worked when I have submitted my patch set on bug 11395. (In reply to Frédéric Demians from comment #21) > Created attachment 37636 [details] [review] [review] > Proposal to clear the job session data at the end of the process Note that I have c/p my test plan to your commit message. (In reply to Jonathan Druart from comment #23) > > And, yes, I confirm that progress bar is also broken in batch item > > modification. It may works that way since a while. > > That's really weird, I'm sure it worked when I have submitted my patch set > on bug 11395. It does not work on my dev install, but works perfectly on sandboxes (tested with batch records modification, and import). I would suspect the perl version (5.20.1 vs 5.14.2). (In reply to Jonathan Druart from comment #23) > I have tried to add this line in the finish method, but did not work. As I understand background process, the finish() method is called when the job is achieved. Then the job result is stored in the session. And from there, the result can be retrieved by a web page (like batch modification page). So the job data clearing must separated from marking a job as finished. If you add a function to a C4 library, you need to add corresponding tests in a .t file too. (In reply to M. Tompsett from comment #27) > If you add a function to a C4 library, you need to add corresponding tests > in a .t file too. Is there somewhere a form to fill out in three copies? Created attachment 37648 [details] [review] Bug 13606 New test for added method to C4::BackgroundJob (In reply to Frédéric Demians from comment #28) > Is there somewhere a form to fill out in three copies? No, just trying to keep the quality high. I see that you encountered the same kind of problem I did when trying to make the test. Here's the problem that still exists. $ mysql -u root -p > use koha_library; > delete from sessions; ^Z $ prove -v t/db_dependent/BackgroundJob.t $ fg > select a_session from sessions where a_session like '%complete%'; ... There are still LOTS of anonymous sessions created for each stage tested in the file. That doesn't seem correct to me. Because if I run it from within Koha, there are not. And from within Koha clear() works, but in the test context it does not. The fail is for a test that doesn't successfully test clear(). Perhaps clear() needs to actually clear based on jobID? I apologize for being a testing pain. I have a patch for tests, will be ready soon. Created attachment 37658 [details] [review] Bug 13606: Add tests and fix existing ones Some tests was wrong and not executed in a transaction. Lot of sessions were created in DB because the session was never flushed. Created attachment 37660 [details] [review] Bug 13606: Add tests and fix existing ones Some tests were wrong and not executed in a transaction. Lot of sessions were created in DB because the session was never flushed. Jonathan: +1 for testing correctly Mark: +1 for keeping the quality high, or elevating it even Created attachment 37662 [details] [review] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I confirm that with more than 1000 records the session object is truncated, and then can't be anymore be desirialized, blocking completely Koha. Fixed with this patch. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> This only pushes off the problem expontentially (64KB vs. 16MB), but this is a good start. Created attachment 37663 [details] [review] Bug 13606 Clear job data in session when the job is completed Bug 11395 uses the session to store the report sent to the interface (update on biblio XX OK, etc.). If the session is stored in mysql, the max size of the sessions.a_session will be reached easily (TEXT field). To reproduce: 0/ Set SessionStorage to 'mysql' 1/ Create a file with 500 biblionumbers: mysql -e "select biblionumber from biblio limit 500;" | tail -n 500 > /tmp/biblionumbers.txt 2/ Define a marc modification template (something like "delete field 99$9" is nice) 3/ Load the /tmp/biblionumbers.txt in the batch biblio modification tool 4/ Repeat 3 You will get: Syck parser (line 1534, column 6): syntax error at /usr/lib/i386-linux-gnu/perl5/5.20/YAML/Syck.pm line 75. None Koha page is reachable. It comes from get_template_and_user > checkauth > get_session > l.1595 $session = new CGI::Session("driver:MySQL;serializer:yaml;id:md5", $sessionID, {Handle=>$dbh}); 5/ Get your sessionID contained in your CGISESSID cookie Have a look at the value a_session in the sessions table: mysql> select a_session from sessions where id="YOUR_SESSIONID"; You should see that the yaml is not correctly ended. The size of the DB field has been reached and the yaml is truncated. Test plan: 0/ Delete your CGISESSID cookie 1/ Try to reproduce the previous issue 2/ After the second batch, have a look at the sessions table and confirm that only one job_$JOB_ID exist in the yaml Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> NOTE: Nicely clears batch job session information. (2) is incorrect. This cleans up after a full run. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37664 [details] [review] Bug 13606: Add tests and fix existing ones Some tests were wrong and not executed in a transaction. Lot of sessions were created in DB because the session was never flushed. TEST PLAN --------- $ mysql ... > delete from sessions; ^Z $ prove -v t/db_dependent/BackgroundJob.t ... $ fg > select id from sessions; -- There will be lots of ids (10, if I recall) ^Z $ git bz apply 13606 ... $ prove -v t/db_dependent/BackgroundJob.t ... $ fg > select * from sessions; -- there should only be the one orphaned job. You can tell it is correct by the "making new job". > quit Run the koha-qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> This is so ready for QA now. :) Thanks everyone. Created attachment 38058 [details] [review] [PASSED QA] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I confirm that with more than 1000 records the session object is truncated, and then can't be anymore be desirialized, blocking completely Koha. Fixed with this patch. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> This only pushes off the problem expontentially (64KB vs. 16MB), but this is a good start. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38059 [details] [review] [PASSED QA] Bug 13606: Upgrade sessions.a_session to MEDIUMTEXT The previous patch fixed partially the problem. If you try to update more that 1000 records, you will get the same error. This patch change the type of sessions.a_session from TEXT (64kb) MEDIUMTEXT (32mb). Test plan: 1/ Try to modify a huge amount of records (1000 should be enough) 2/ Confirm it worked! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I confirm that with more than 1000 records the session object is truncated, and then can't be anymore be desirialized, blocking completely Koha. Fixed with this patch. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> This only pushes off the problem expontentially (64KB vs. 16MB), but this is a good start. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38060 [details] [review] [PASSED QA] Bug 13606 Clear job data in session when the job is completed Bug 11395 uses the session to store the report sent to the interface (update on biblio XX OK, etc.). If the session is stored in mysql, the max size of the sessions.a_session will be reached easily (TEXT field). To reproduce: 0/ Set SessionStorage to 'mysql' 1/ Create a file with 500 biblionumbers: mysql -e "select biblionumber from biblio limit 500;" | tail -n 500 > /tmp/biblionumbers.txt 2/ Define a marc modification template (something like "delete field 99$9" is nice) 3/ Load the /tmp/biblionumbers.txt in the batch biblio modification tool 4/ Repeat 3 You will get: Syck parser (line 1534, column 6): syntax error at /usr/lib/i386-linux-gnu/perl5/5.20/YAML/Syck.pm line 75. None Koha page is reachable. It comes from get_template_and_user > checkauth > get_session > l.1595 $session = new CGI::Session("driver:MySQL;serializer:yaml;id:md5", $sessionID, {Handle=>$dbh}); 5/ Get your sessionID contained in your CGISESSID cookie Have a look at the value a_session in the sessions table: mysql> select a_session from sessions where id="YOUR_SESSIONID"; You should see that the yaml is not correctly ended. The size of the DB field has been reached and the yaml is truncated. Test plan: 0/ Delete your CGISESSID cookie 1/ Try to reproduce the previous issue 2/ After the second batch, have a look at the sessions table and confirm that only one job_$JOB_ID exist in the yaml Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> NOTE: Nicely clears batch job session information. (2) is incorrect. This cleans up after a full run. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38061 [details] [review] [PASSED QA] Bug 13606: Add tests and fix existing ones Some tests were wrong and not executed in a transaction. Lot of sessions were created in DB because the session was never flushed. TEST PLAN --------- $ mysql ... > delete from sessions; ^Z $ prove -v t/db_dependent/BackgroundJob.t ... $ fg > select id from sessions; -- There will be lots of ids (10, if I recall) ^Z $ git bz apply 13606 ... $ prove -v t/db_dependent/BackgroundJob.t ... $ fg > select * from sessions; -- there should only be the one orphaned job. You can tell it is correct by the "making new job". > quit Run the koha-qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38431 [details] [review] Bug 13606: (RM followup) DBIx updates Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Patches pushed to master. Thanks Jonathan and Frederic! Doesn't apply and I think is not needed on 3.18.x (In reply to Chris Cormack from comment #45) > Doesn't apply and I think is not needed on 3.18.x No it's not, bug 11395 will be in 3.20 |