Refactoring on index benchmark execution and ControlledExecutor #50

patsonluk · 2023-02-15T03:22:39Z

Description

A refactoring effort to address:

duration-sec is defined for IndexBenchmark but it currently has no effect
Indexing test uses its own implementation for rate control, and does not share the ControlledExecutor which controls duration/rate for querying test
ControlledExecutor has certain discrepancy for the rpm configured vs the actual rpm achieved especially in high throughput scenarios

Solution

Introduced IndexBatchSupplier which "supply" tasks (Callable) which each callable is a index batch writing operation, refactored existing indexing logic to use such supplier and pass that to ControlledExecutor. Such IndexBatchSupplier ensures better separation of concerns of the 2 major tasks:
1. the docs reading/parsing part from the input doc json file (as DocReader/FileDocReader that respect maxDocs)
2. the indexing task generation (look up shard, forming batch etc)
Simplified and rewrote some logic in ControlledExecutor such that the controls (duration/rpm) are more well defined and precisely respected.
Simplified RateLimiter to regulate actual rpm closer to the target rpm provided

This is a pretty big refactoring, but I'm hoping this could simplify/solidify the query/index design and code implementation. Could really use some code review and thoughts! @chatman Many thanks! 🙇🏼

Simplified some logic in ControlledExecutor such that it can better adheres to the rpm/max execution count defined

chatman

I haven't tested these changes, but nothing jumps out as obviously wrong. Thanks for tackling this.

patsonluk · 2023-02-15T17:07:06Z

I haven't tested these changes, but nothing jumps out as obviously wrong. Thanks for tackling this.

Thank you so much @chatman! This is one sizeable refactoring so take your time!

I don't have time yet to write any unit test cases 😓 but I did some simple testing on my end (mostly single threaded tho):

Test on having different combination of the restriction factors - duration, maxDocs/runCount. Verified the expected behavior
Verified the rpm rate after the changes.
Ensure index tasks still fulling index all docs when no restrictions are given

I have also started using that for our use case - having light indexing and heavy querying concurrently with fixed duration for both.

Would be great if you can test on your use cases as well! 💪🏼

chatman · 2023-02-15T17:17:24Z

Sure, I'll update you by tomorrow on how the testing goes. THanks!

patsonluk · 2023-02-17T20:05:22Z

@chatman thanks for approving it! Im wondering if you have a chance to run through the test cases on ur end too? 😊

chatman · 2023-02-18T14:01:37Z

Im wondering if you have a chance to run through the test cases on ur end too?

I'll test them today. Thank your for the patience :-)

noblepaul · 2023-02-20T11:08:52Z

src/main/java/org/apache/solr/benchmarks/indexing/DocReader.java

+   * @return
+   * @throws IOException
+   */
+  List<String> readDocs(int count) throws IOException;


nextBatch() would be better name?

noblepaul · 2023-02-20T11:09:53Z

src/main/java/org/apache/solr/benchmarks/indexing/DocReader.java

+import java.io.IOException;
+import java.util.List;
+
+public interface DocReader extends AutoCloseable {


This is actually not a DocReader. This is agnostic of docs. it is a BatchedLinesReader
or we should get rid of this interface altogether and use Iterator<List<String>>

Thank you for bringing that up!

I named it DocReader as it's specific to our usage (reading docs as lines of string) even though it looks pretty much like a BatchedLinesReader as u mention. The implementing class does expect to read in lines and perform any pre-processing (remove _version_ for example) such that the line read are ready to be consumed as Doc as is.

Probably cannot just use Iterator as it's a bit too generic, code reader could be hard to reason what it exactly is and this interface does more than just iteration (for example reading a batch of input docs) and could have extra methods added for other doc specific operations in the future :)

noblepaul · 2023-02-20T11:14:59Z

src/main/java/org/apache/solr/benchmarks/indexing/IndexBatchSupplier.java

+
+        @Override
+        public void handle(Map<String, Object> record, String path) {
+            idParsed = record.get(benchmark.idField) instanceof String ?


if idField is missing you may get an NPE

AFAIK, the bean should be idField defeault as "id". This Parser is refactoring of existing code from https://github.com/fullstorydev/solr-bench/pull/50/files/07565fdeb86670e2e32057cb893d0605a836a14e#diff-c443b311a116a519dcdbf35939db1b752571af41e44a38f37368dc4770092ab1L318 so it should be rather safe 😊

noblepaul · 2023-02-20T11:15:29Z

src/main/java/org/apache/solr/benchmarks/indexing/IndexBatchSupplier.java

+                while (!exit && (inputDocs = docReader.readDocs(benchmark.batchSize)) != null) { //can read more than batch size, just use batch size as a sensible value
+                    for (String inputDoc : inputDocs) {
+                        rdr.streamRecords(new StringReader(inputDoc), idParser);
+                        Slice targetSlice = docCollection.getRouter().getTargetSlice(idParser.idParsed, null, null, null, docCollection);


handle missing id field?

noblepaul · 2023-02-20T11:16:18Z

src/main/java/org/apache/solr/benchmarks/indexing/IndexBatchSupplier.java

+                    for (String inputDoc : inputDocs) {
+                        rdr.streamRecords(new StringReader(inputDoc), idParser);
+                        Slice targetSlice = docCollection.getRouter().getTargetSlice(idParser.idParsed, null, null, null, docCollection);
+                        List<String> shardDocs = shardVsDocs.get(targetSlice.getName());


computeIfAbsent() ?

Good suggestion! 👍🏼

noblepaul · 2023-02-20T11:19:05Z

src/main/java/org/apache/solr/benchmarks/indexing/IndexBatchSupplier.java

+                        }
+                    }
+                }
+                shardVsDocs.forEach((shard, docs) -> { //flush the remaining ones


so, accordig to this logic , if shard1 has crossed the batch size, all shards are uploaded? is this the intent?

Great questions!

This is to mimic the old behavior at https://github.com/fullstorydev/solr-bench/pull/50/files/07565fdeb86670e2e32057cb893d0605a836a14e#diff-c443b311a116a519dcdbf35939db1b752571af41e44a38f37368dc4770092ab1L358

which after all lines are read from the source, it would flush out whatever is left that are added to shardVsDocs map, yet didn't pushed out as it has not reached the batch size. Take note that the logic here would only send out the rest if exit is not true (ie noone has called close() on this supplier yet). Therefore we can break in down into below scenarios:

If noone has explicitly called close, the reader would just read all the lines and this code in IndexBatchSupplier would flush the remaining docs in shardVsDocs

If someone has called close, IndexBatchSupplier should not attempt to queue any more pending batches. Take note that no current code would do this while the supplier is active. This is just to put into place for the completeness of implementation (and probably some future usage can make use of this)

Take note that this supplier is unaware of duration/maxExecution or maxDocs settings for the benchmark. Such 2 restrictions are handled elsewhere:

duration/maxExecution by ControlledExecuctor, such executor can decide to drop the remaining task (if duration elapsed) or continue with the rest of the submitted batch (if submitted count reaches maxExecution)

maxDocs is handled by the DocReader, once it reaches the maxDocs, readDocs method should return null

chatman · 2023-02-21T14:09:08Z

@patsonluk , I ran them on a few suites yesterday and didn't notice any problems.

chatman · 2023-02-21T14:10:53Z

I don't have time yet to write any unit test cases

I've created an issue here, we can revisit later.
#53

… the exception bubble up and the main flow to handle it (interrupt corresponding tasks)

This reverts commit 0e93b21.

patsonluk added 7 commits February 14, 2023 18:49

Refactoring on indexing to use ControlledExecutor.

8f01720

Simplified some logic in ControlledExecutor such that it can better adheres to the rpm/max execution count defined

Fix NPE if maxDocs is not defined

949aba6

Code cleanup

ec09682

Better logging for ControlledExecutor

39e300e

Fixed recurring EOFException

4abd3e6

Better implementation handle duration restrictions

6eda06a

Better implementation handle duration restrictions

b2e0b34

chatman approved these changes Feb 15, 2023

View reviewed changes

Base automatically changed from patsonluk/benchmark-bean-refactoring to master February 15, 2023 16:58

Code cleanup

07565fd

noblepaul reviewed Feb 20, 2023

View reviewed changes

patsonluk added 7 commits February 21, 2023 13:17

Update after code review

6132722

Merge branch 'master' into patsonluk/controlled-index-benchmark

c295756

Remove try catch all Exception on runIndexingBenchmarks. Should let…

7bae520

… the exception bubble up and the main flow to handle it (interrupt corresponding tasks)

Changed default query shuffle to false

0e93b21

Revert "Changed default query shuffle to false"

8952751

This reverts commit 0e93b21.

Exception handling

c1364b0

Improved back pressure handling

d9ce455

patsonluk merged commit df09bf2 into master Feb 27, 2023

patsonluk deleted the patsonluk/controlled-index-benchmark branch February 27, 2023 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring on index benchmark execution and ControlledExecutor #50

Refactoring on index benchmark execution and ControlledExecutor #50

patsonluk commented Feb 15, 2023

chatman left a comment

patsonluk commented Feb 15, 2023

chatman commented Feb 15, 2023

patsonluk commented Feb 17, 2023

chatman commented Feb 18, 2023

noblepaul Feb 20, 2023

noblepaul Feb 20, 2023 •

edited

Loading

patsonluk Feb 21, 2023

noblepaul Feb 20, 2023

patsonluk Feb 21, 2023

noblepaul Feb 20, 2023

noblepaul Feb 20, 2023

patsonluk Feb 21, 2023

noblepaul Feb 20, 2023

patsonluk Feb 21, 2023 •

edited

Loading

chatman commented Feb 21, 2023

chatman commented Feb 21, 2023

Refactoring on index benchmark execution and ControlledExecutor #50

Refactoring on index benchmark execution and ControlledExecutor #50

Conversation

patsonluk commented Feb 15, 2023

Description

Solution

chatman left a comment

Choose a reason for hiding this comment

patsonluk commented Feb 15, 2023

chatman commented Feb 15, 2023

patsonluk commented Feb 17, 2023

chatman commented Feb 18, 2023

Choose a reason for hiding this comment

noblepaul Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patsonluk Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

chatman commented Feb 21, 2023

chatman commented Feb 21, 2023

noblepaul Feb 20, 2023 •

edited

Loading

patsonluk Feb 21, 2023 •

edited

Loading