Bug 26996 - Elasticsearch: Multiprocess reindexing sometimes doesn't reindex all records
Summary: Elasticsearch: Multiprocess reindexing sometimes doesn't reindex all records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Björn Nylén
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-11 10:50 UTC by Björn Nylén
Modified: 2021-12-13 21:09 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02,20.05.08
Circulation function:


Attachments
Truncate commit size to make sure it's an integer (1.47 KB, patch)
2020-11-11 11:20 UTC, Björn Nylén
Details | Diff | Splinter Review
Truncate commit size to make sure it's an integer - QA update (1.59 KB, patch)
2020-12-07 08:17 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 26996: Convert Elasticsearch indexer commit buffer size to integer (1.74 KB, patch)
2020-12-07 13:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26996: Convert Elasticsearch indexer commit buffer size to integer (1.79 KB, patch)
2020-12-23 15:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Nylén 2020-11-11 10:50:58 UTC
We've discovered that rebuild_elasticsearch.pl will not index all records in some cases using multiple cpu's. Sometimes one (or more) child proc. will loop through their records without ever committing them to ES.

Symptoms are that the process will work faster than others, not log "Committing xxx records" and consume ever growing amount of memory. 

The problem appears to be in the fudging of commmit sizes for the childs. It generates a float which will in some cases never be == 0 as the buffer counter is decreased.

This set of param will fail for me everytime
./rebuild_elasticsearch -v -b -p 2 -c 400

Will uplaod a patch shortly.
Comment 1 Björn Nylén 2020-11-11 11:20:24 UTC
Created attachment 113482 [details] [review]
Truncate commit size to make sure it's an integer

Patch to truncate commit size to be sure it's an integer

To test you must find a set of circumstances that causes the issue. For me:
1. Run: ./rebuild_elasticsearch -v -b -p 2 -c 400
2. Note that only one process is logging "Committing xxx records..."
3. Kill processes.
4. Apply patch.
5. Repeat 1
6. Note that only both processes are logging "Committing xxx records..."
Comment 2 Joonas Kylmälä 2020-12-04 14:14:11 UTC
Thanks, this fixes the problem indeed. Few things to improve on the patch:

- In the commit title describe what the fix does, not what the problem is. E.g. "Convert Elasticsearch indexer commit buffer size to int" and in the body describe that it fixes the buffer counter not resetting and the reason why it fixes it.

- In the commit title use "Bug XXXX: YYY" format which is used by every other commit we have (now there is the "-" character instead).

- Add whitespace between the multiplication and parentheses.
Comment 3 Björn Nylén 2020-12-07 08:17:46 UTC
Created attachment 114227 [details] [review]
Truncate commit size to make sure it's an integer - QA update

New patch with suggested QA changes. Moved the truncation to a separeate line to make it more obvious.
Comment 4 Joonas Kylmälä 2020-12-07 13:44:09 UTC
Created attachment 114232 [details] [review]
Bug 26996: Convert Elasticsearch indexer commit buffer size to integer

When multithreaded indexing is used, the commit size for children are spread
out resulting in them being of type float. When records are processed and the
commit counter decreased it may then never reach *exactly* 0. This means records
are never commited. This patch makes sure the counter is an integer to avoid the
problem.

To test you must find a set of circumstances that causes the issue. For me:
1. Run: ./rebuild_elasticsearch -v -b -p 2 -c 400
2. Note that only one process is logging "Committing xxx records..."
3. Kill processes.
4. Apply patch.
5. Repeat 1
6. Note that both processes are logging "Committing xxx records..."

Sponsored-by: Lund University Library
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 5 Nick Clemens (kidclamp) 2020-12-23 15:40:45 UTC
Created attachment 114679 [details] [review]
Bug 26996: Convert Elasticsearch indexer commit buffer size to integer

When multithreaded indexing is used, the commit size for children are spread
out resulting in them being of type float. When records are processed and the
commit counter decreased it may then never reach *exactly* 0. This means records
are never commited. This patch makes sure the counter is an integer to avoid the
problem.

To test you must find a set of circumstances that causes the issue. For me:
1. Run: ./rebuild_elasticsearch -v -b -p 2 -c 400
2. Note that only one process is logging "Committing xxx records..."
3. Kill processes.
4. Apply patch.
5. Repeat 1
6. Note that both processes are logging "Committing xxx records..."

Sponsored-by: Lund University Library
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2020-12-23 15:42:00 UTC
In testing it did seem as if the records were committed in the end, caught by the final index of remaining records, but very not ideal, especially in a large system
Comment 7 Jonathan Druart 2021-01-04 13:10:31 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 8 Fridolin Somers 2021-01-07 13:52:43 UTC
Pushed to 20.11.x for 20.11.02
Comment 9 Andrew Fuerste-Henry 2021-01-11 21:14:57 UTC
Pushed to 20.05.x for 20.05.08
Comment 10 Victor Grousset/tuxayo 2021-01-18 18:37:29 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.