I would prefer kconf so that test env with same database can be different.

We usually have prod using real cluster and test env using a single node side-container.

Note: this patch doesn't affect the command line indexing script which allows you to pass a commit size defining how many records to send

I've signed off, as everything in the test plan worked.

However, I did note one thing when updating authority records:

1. Starting fresh (patch applied, shutting down KTD, then starting up again so that there are no previous jobs, adding the <chunk_size>250</chunk_size>, restarting everything).

2. After updating all the authority records by adding text to 680$1, there are 1320 entries for jobs:
   - the last 7 job entries (1314-1320) are for the elastic search index updates, split into 250 chunks (1320 is for 206 record updates)
   - the first job entry (1) is for the batch authority record modification (1706 modifications)
   - the rest of the job entries (2-1313) are for Elasticsearch index updates to individual bibliographic records (from a sample I checked)

3. I'm assuming that the individual bibliographic record updates are because the authority terms updated are linked to them.

4. I don't know whether this is what is expected, or whether there are plans to chunk the subsequent individual bibliographic records updated because of authority term changes. Or whether that is even possible.

5. Irrespective of that, this is still a great improvement!

Testing notes (using KTD):

1. I tested using ES8 (ktd --es8 up).

2. For the modification of bibliographic records, I updated the 'z - Public note' with some text.

3. For the modification of authority records, I had a rule to add some text to 680$i subfield.

Created attachment 160530 [details] [review]
Bug 35086: Add chunk_size option to elasticsearch configuration

Whne performing batch operations we can send a large numebr of records for reindexing at once.
Currently this can create requetss that are too large for Elasticsearch to process. We need
to break these requests into chunks/

This patch adds a chunk_size configuration to the elasticsearch stanza in koha-conf.xml

If blank we default to 5000.

To test:
0 - Have Koha using Elasticsearch
1 - Create and download a report of all barcodes:
    SELECT barcode FROM items
2 - Batch modify these items
3 - Note a single ESindexing job is created
4 - Create and download a report of all authority ids:
    SELECT auth_header.authid FROM auth_header
5 - Setup a marc modification template, and batch modify all the authorities
6 - Again note a single ES backgorund job is created
7 - Apply patch
8 - Repeat the modifications above - you still get a single job
9 - Edit koha-conf.xml and add <chunk_size>250</chunk_size> to elasticsearch stanza
10 - Repeat modifications - you now get several background ES jobs
11 - prove -v t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160531 [details] [review]
Bug 35086: Also split chunks when indexing from background job

The es background indexer is designed to combine background jobs when started based on the 'batch_size' option.

While this is helpful for combining individual updates, it can be problematic when there are several large batch modifications, or when worker has stopped and is restarted.

This patch uses the same logic as in the indexer to split the chunks that are sent directly for indexing.

To test:
1 - Follow test plan on previous patch
2 - Confirm items are correctly indexed and jobs marked

Created attachment 160532 [details] [review]
Bug 35086: Tidy tests

Created attachment 160560 [details] [review]
Bug 35086: Add chunk_size option to elasticsearch configuration

Whne performing batch operations we can send a large numebr of records for reindexing at once.
Currently this can create requetss that are too large for Elasticsearch to process. We need
to break these requests into chunks/

This patch adds a chunk_size configuration to the elasticsearch stanza in koha-conf.xml

If blank we default to 5000.

To test:
0 - Have Koha using Elasticsearch
1 - Create and download a report of all barcodes:
    SELECT barcode FROM items
2 - Batch modify these items
3 - Note a single ESindexing job is created
4 - Create and download a report of all authority ids:
    SELECT auth_header.authid FROM auth_header
5 - Setup a marc modification template, and batch modify all the authorities
6 - Again note a single ES backgorund job is created
7 - Apply patch
8 - Repeat the modifications above - you still get a single job
9 - Edit koha-conf.xml and add <chunk_size>250</chunk_size> to elasticsearch stanza
10 - Repeat modifications - you now get several background ES jobs
11 - prove -v t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160561 [details] [review]
Bug 35086: Also split chunks when indexing from background job

The es background indexer is designed to combine background jobs when started based on the 'batch_size' option.

While this is helpful for combining individual updates, it can be problematic when there are several large batch modifications, or when worker has stopped and is restarted.

This patch uses the same logic as in the indexer to split the chunks that are sent directly for indexing.

To test:
1 - Follow test plan on previous patch
2 - Confirm items are correctly indexed and jobs marked

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160562 [details] [review]
Bug 35086: Tidy tests

Signed-off-by: David Nind <david@davidnind.com>

Here are the list of jobs from testing using a freshly started KTD:

0000-1316 - Before patch, for both item modification and authority 
            record changes
1317-1318 - After patch, no chunking, item modifications
1319-2632 - After patch, no chunking, authority record changes
            ==> no change (as expected) to number of jobs after patch
                applied and no chunking set
2633-2635 - After patch, chunking (250), item modifications
            . 1 job for batch item modifications, 2 jobs for elastic search
              updates (1 batch of 250 and 1 batch of 161)
2636-3955 - After patch, chunking (250), authority record changes
            . 2636 - Batch authority record modification
            . 2637-3948 - Elasticsearch index updates for 1,312 individual
                          bibliographic record updates
            . 3949-3955 - 6 batches of 250, 1 batch of 206

I'm assuming this is what is expected, feel free to change the bug status if it isn't.


Testing notes:

1. I tested using ES8 (ktd --es8 up).

2. For the modification of bibliographic records, I updated the 'z - Public note' with some text.

3. For the modification of authority records, I had a rule to add some text to 680$i subfield.

you have 500 in conf and 5000 in pm, is that expected?

Created attachment 160572 [details] [review]
Bug 35086: (follow-up) Use 5000 as example in conf file

(In reply to Jonathan Druart from comment #13)
> you have 500 in conf and 5000 in pm, is that expected?

500 seemed a more reasonable size in my head, but 5000 is more consistent with our default indexing so I updated it.

I am wondering about the changes made to the worker.

+        while ( ( my @auth_chunk = $auth_chunks->() ) ) {
+            try {
+                $auth_indexer->update_index( \@auth_chunk );
+            } catch {
+                $logger->warn( sprintf "Update of elastic index failed with: %s", $_ );
+            };
+        }

Should not we surround the while with a try instead?... Not sure what's best here!

(In reply to Jonathan Druart from comment #16)
> Should not we surround the while with a try instead?... Not sure what's best
> here!

I'd rather try to index what we can, and only fail the bits that didn't work - i.e. if we have a big job and encounter an error early - a try on the whole thing would fail on the first chunk and stop. This way it tries each chunk - so one might fail, but the rest succeed - if there are errors lets minimize what needs to be reindexed

Created attachment 160853 [details] [review]
Bug 35086: Add chunk_size option to elasticsearch configuration

Whne performing batch operations we can send a large numebr of records for reindexing at once.
Currently this can create requetss that are too large for Elasticsearch to process. We need
to break these requests into chunks/

This patch adds a chunk_size configuration to the elasticsearch stanza in koha-conf.xml

If blank we default to 5000.

To test:
0 - Have Koha using Elasticsearch
1 - Create and download a report of all barcodes:
    SELECT barcode FROM items
2 - Batch modify these items
3 - Note a single ESindexing job is created
4 - Create and download a report of all authority ids:
    SELECT auth_header.authid FROM auth_header
5 - Setup a marc modification template, and batch modify all the authorities
6 - Again note a single ES backgorund job is created
7 - Apply patch
8 - Repeat the modifications above - you still get a single job
9 - Edit koha-conf.xml and add <chunk_size>250</chunk_size> to elasticsearch stanza
10 - Repeat modifications - you now get several background ES jobs
11 - prove -v t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 160854 [details] [review]
Bug 35086: Also split chunks when indexing from background job

The es background indexer is designed to combine background jobs when started based on the 'batch_size' option.

While this is helpful for combining individual updates, it can be problematic when there are several large batch modifications, or when worker has stopped and is restarted.

This patch uses the same logic as in the indexer to split the chunks that are sent directly for indexing.

To test:
1 - Follow test plan on previous patch
2 - Confirm items are correctly indexed and jobs marked

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 160855 [details] [review]
Bug 35086: Tidy tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 160856 [details] [review]
Bug 35086: (follow-up) Use 5000 as example in conf file

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed for 24.05!

Well done everyone, thank you!

Tweaked the release notes text - feel free to improve!

Pushed to 23.11.x for 23.11.02

Backported to 23.05.x for upcoming 23.05.09