Bug 26996

Summary:	Elasticsearch: Multiprocess reindexing sometimes doesn't reindex all records
Product:	Koha	Reporter:	Björn Nylén <bjorn.nylen>
Component:	Searching - Elasticsearch	Assignee:	Björn Nylén <bjorn.nylen>
Status:	CLOSED FIXED	QA Contact:
Severity:	minor
Priority:	P5 - low	CC:	andrew, caroline.cyr-la-rose, fridolin.somers, joonas.kylmala, nick, victor
Version:	Main
Hardware:	All
OS:	All
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 Truncate commit size to make sure it's an integer - QA update Bug 26996: Convert Elasticsearch indexer commit buffer size to integer Bug 26996: Convert Elasticsearch indexer commit buffer size to integer

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.