IncrementalIndex Tests and Benchmarks Parametrization #10593

liran-funaro · 2020-11-18T13:56:44Z

Description

Note: This PR only affects tests and benchmarks.
It would help developers evaluate incremental-index extensions, such as oak-incremental-index (#10001).

#10335 added a per incremental-index builder, but the parent class builder (IncrementalIndex.Builder) was not removed to avoid 100+ line changes in the test code.
This PR removes IncrementalIndex.Builder and refactor all its usage (only in tests the test/benchmarks code).
In addition, where needed, a parametrization was added so it will test/benchmark both builder implementations (on-heap and off-heap).

Add test cases for each index type

All tests that are relevant to the incremental index were modified. The modifications include the parametrization of the tests for all incremental-index implementations: on-heap and off-heap. In addition, this PR includes a bug fix in OffheapIncrementalIndex that was found using these tests.

To support this, a new helper class was added: IncrementalIndexCreator.
This class handle creating the appropriate index according to its name and closing it at the end of each test.

Add benchmark cases for each index type

All the benchmarks that are relevant to the incremental index were added an incremental-index parametrization: on-heap or off-heap.
In addition, some of these benchmarks were modified to resolve some issues that were encountered.

We list here the additional modifications we made to some of the benchmarks.

Add some additional parametrization:
- rollup opportunity for the row generator
- number of rows per segment
- query order: descending/ascending
Add a missing tearDown() procedure
Properly close the queryable index in the tearDown() procedure
Moved any temporary folder creation and deletion to the setup()/tearDown() methods so they would not affect the measurements of the results
Use a predefined seed for reproducible results, to be compliant with most benchmarks
Add scopes (@State(Scope.Benchmark)) that allow us to test the incremental index without the overhead of the setup procedure of the queryable index benchmark
- One scope for benchmarking queries on the incremental index
- One scope for benchmarking queries on the queryable index

In addition, to reduce code duplications, a few methods were added to DataGenerator:

void addToIndex(IncrementalIndex<?> index, int numOfRows): adds rows from this generator to an existing index
List<InputRow> toList(int numOfRows): adds rows from this generator to a new list

User Experience Changes

After this PR, the user should not expect changes in most benchmarks results.
However, some benchmarks behavior will change as follows:

Runtime

Expected change: the following benchmarks will run much faster due to eliminating redundancy setup/teardown procedure calls.
However, the benchmarks reported results should not change.
FilteredAggregatorBenchmark, GroupByBenchmark, ScanBenchmark, SearchBenchmark, TimeseriesBenchmark, TopNBenchmark

Parametrization

Expected change: the following benchmarks will have additional parametrization options, hence they might take longer to run and produce more results.

indexType parametrization (will also test the off-heap implementation): FilteredAggregatorBenchmark, IncrementalIndexRowTypeBenchmark, IncrementalIndexReadBenchmark, IndexIngestionBenchmark, IndexPersistBenchmark, GroupByBenchmark, ScanBenchmark, SearchBenchmark, TimeseriesBenchmark, TopNBenchmark
descending query parametrization: FilteredAggregatorBenchmark, TimeseriesBenchmark
rollupOpportunity ingestion parametrization: IndexIngestionBenchmark

Unified Benchmarks Behaviour

Expected change: these changes affect some of the benchmarks' reported results as follows:

A rowsPerSegment parametrization was added to IncrementalIndexRowTypeBenchmark. Before the number of rows was not parametrized and it reported the time per single row insertion. Now it reports the total insertion time of all the rows, like the rest of the tests report.
ScanBenchmark, SearchBenchmark, GroupByBenchmark: now using a fixed seed, so the results are reproducible but might be slightly different than what was before with the random seed.
IndexPersistBenchmark: this benchmark previously cleaned the temporary data folder inside the tested method, instead of in the teardown procedure. For large index sizes, it affected the benchmark result significantly. With this modification, the results will be different (shorter times), but it will better reflect the "persist" performance.

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

Eshcar

Thank Liran for this PR.

It is generally a very big PR.
The text description helps a lot in reviewing the code.
However, there are some classes with very big changes, and it is not always easy to track back the reason for these changes, see my comments below.
Adding (many) lines of documentation within the code can greatly help the next reviewers read and approve the code.

Eshcar · 2020-11-29T14:06:33Z

benchmarks/src/test/java/org/apache/druid/benchmark/FilterPartitionBenchmark.java

@@ -227,10 +228,10 @@ public void tearDown() throws IOException

  private IncrementalIndex makeIncIndex()
  {
-    return new IncrementalIndex.Builder()
+    return new OnheapIncrementalIndex.Builder()


shouldn't this method take a parameter to decide which type of index to return?
or is this the default builder?
then maybe buildDefaultIncIndex and the default should be some hard coded value that can be changed over time

I agree. But for the sake of reducing the diff size, I'd prefer to avoid this refactor.

Eshcar · 2020-11-29T14:13:42Z

benchmarks/src/test/java/org/apache/druid/benchmark/FilteredAggregatorBenchmark.java

  {
-    FileUtils.deleteDirectory(tmpDir);


the diff here is very misleading - this line is part of a one line method tearDown that was deleted

Note that it was not deleted. It just moved below to the teardown of the QueryableIndexState: qIndexesDir.delete();

Eshcar · 2020-11-29T14:17:21Z

benchmarks/src/test/java/org/apache/druid/benchmark/FilteredAggregatorBenchmark.java

+  public static class IncrementalIndexState
+  {
+    @Param({"onheap", "offheap"})
+    private String indexType;


since now there is a new extension point for incremental index, shouldn't the type be extendable as well?
use enum instead of string and names like defaultOnHeap and OakOffHeap so additional on/off-heap implementations can be added in the future

The idea here is indeed to allow a future index to be tested with the same code.
Using Enum will force this enumeration to list all existing index types in the core Druid package, albeit the index may only exist as an extension.
This way (using string), the user can choose any indexType name in the command line without it having to be pre-defined in the code.

Eshcar · 2020-11-29T14:21:50Z

benchmarks/src/test/java/org/apache/druid/benchmark/FilteredAggregatorBenchmark.java

+    @TearDown
+    public void tearDown()
+    {
+      qIndex.close();


no option for qIndex to be null? e.g., if indexFile is empty?

I'm not sure. But if setup fails, this might be an issue. Added a null check just to be safe.

Eshcar · 2020-11-29T14:52:36Z

benchmarks/src/test/java/org/apache/druid/benchmark/IncrementalIndexRowTypeBenchmark.java

  public void setup2()
  {
    incIndex = makeIncIndex();
-    incFloatIndex = makeIncIndex();


Does adding all rows into one index equivalent to having 3 indices?

Notice that the setup level was changed to per invocation, so for each benchmark, a new index is created.
There wasn't really a need for three different indices in the first place.

Eshcar · 2020-11-29T15:11:50Z

benchmarks/src/test/java/org/apache/druid/benchmark/query/GroupByBenchmark.java

  {
    log.info("SETUP CALLED AT " + +System.currentTimeMillis());

    ComplexMetrics.registerSerde("hyperUnique", new HyperUniquesSerde());

-    executorService = Execs.multiThreaded(numProcessingThreads, "GroupByThreadPool[%d]");


from this point onward a bit hard to follow the reasoning for the changes? what part of the PR description does this relate to?

These changes are due to the scoping of the benchmark. This setup method now only in charge of initializing everything common for benchmarking both the incremental-index and the queriable-index.
Anything specific to the incremental or queriable index was moved to its designated scope below.

Eshcar · 2020-11-29T15:17:29Z

processing/src/main/java/org/apache/druid/segment/incremental/OffheapIncrementalIndex.java

-        bufferIndex = indexAndOffset[0];
-        bufferOffset = indexAndOffset[1];
-        aggBuffer = aggBuffers.get(bufferIndex).get();
+        ByteBuffer aggBuffer = aggBuffers.get(indexAndOffset[0]).get();


what's the reasoning for these changes? add documentation to explain

Before this change, the code that responsible for the aggregation ran after a new row was inserted to indexAndOffsets (see line 209 below). This means that the new row was visible before any data was aggregated to it.
This does not correspond with the on-heap index behavior, which first aggregates the data, then inserts the row to the index.
According to IncrementalIndexIngestionTest.testMultithreadAddFacts(), the on-heap behavior is the correct one, so I changed it accordingly so the test will pass for this index as well.

Hmm I feel that this fix to OffheapIncrementalIndex can be independent in its own separate issue and PR since it is a bug and would make it easier for tracking in the future.

If this bug fix will be in a separate PR, then this PR will have a failing test.

Eshcar · 2020-11-29T15:20:03Z

processing/src/test/java/org/apache/druid/query/DoubleStorageTest.java

@@ -321,10 +322,10 @@ private static QueryableIndex buildIndex(String storeDoubleAsFloat) throws IOExc
        )
        .build();

-    final IncrementalIndex index = new IncrementalIndex.Builder()


from here on forward same 5 lines changes repeat for different tests

This is true for tests that do not test the incremental-index implementation. For such tests, we use the on-heap implementation because it is the most stable.
Tests that test the index itself, are now parametrized so they have more modifications other than these 5 lines.

Eshcar · 2020-11-29T15:25:05Z

processing/src/test/java/org/apache/druid/segment/data/IncrementalIndexTest.java

@@ -268,7 +217,7 @@ private static MapBasedInputRow getLongRow(long timestamp, int dimensionCount)
  public void testCaseSensitivity() throws Exception
  {
    long timestamp = System.currentTimeMillis();
-    IncrementalIndex index = closerRule.closeLater(indexCreator.createIndex(DEFAULT_AGGREGATOR_FACTORIES));
+    IncrementalIndex<?> index = indexCreator.createIndex((Object) DEFAULT_AGGREGATOR_FACTORIES);


all changes from here are due to the generic type?

Yes. To allow testing any new incremental index.

Thanks for addressing the questions and issues
LGTM

Eshcar · 2020-11-29T15:28:46Z

...sing/src/test/java/org/apache/druid/segment/incremental/OffheapIncrementalIndexTestSpec.java

+import java.nio.ByteBuffer;
+
+/**
+ * Since the off-heap incremental index is not yet supported in production ingestion, we define its spec here only


add a more general documentation of the role of this class for the time it is supported

Eshcar

The changes in this PR would help people evaluate the oak extension.

Eshcar · 2020-12-06T08:16:08Z

processing/src/test/java/org/apache/druid/segment/data/IncrementalIndexTest.java

@@ -268,7 +217,7 @@ private static MapBasedInputRow getLongRow(long timestamp, int dimensionCount)
  public void testCaseSensitivity() throws Exception
  {
    long timestamp = System.currentTimeMillis();
-    IncrementalIndex index = closerRule.closeLater(indexCreator.createIndex(DEFAULT_AGGREGATOR_FACTORIES));
+    IncrementalIndex<?> index = indexCreator.createIndex((Object) DEFAULT_AGGREGATOR_FACTORIES);


Thanks for addressing the questions and issues
LGTM

a2l007

Thanks for the PR. I've left some comments. If you have the results from your local run of the modified benchmarks, could you please post them here?

a2l007 · 2020-12-09T19:40:55Z

processing/src/main/java/org/apache/druid/segment/generator/DataGenerator.java

+  }
+
+  /**
+   * Add rows form any generator to an index.


Typo: form -> from

a2l007 · 2020-12-09T20:26:14Z

processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCreator.java

+  public static AppendableIndexSpec parseIndexType(String indexType) throws JsonProcessingException
+  {
+    return JSON_MAPPER.readValue(
+        String.format(Locale.ENGLISH, "{\"type\": \"%s\"}", indexType),


Can we use StringUtils.format here instead?

processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCreator.java

a2l007 · 2020-12-09T21:34:34Z

processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCreator.java

+   * @param c a list of collections of parameters
+   * @return the cartesian product of all parameters
+   */
+  public static List<Object[]> cartesianProduct(Collection<?>... c)


Does this method need public visibility?

No. Changed to private.

a2l007 · 2020-12-09T22:43:35Z

processing/src/main/java/org/apache/druid/segment/incremental/OffheapIncrementalIndex.java

-        bufferIndex = indexAndOffset[0];
-        bufferOffset = indexAndOffset[1];
-        aggBuffer = aggBuffers.get(bufferIndex).get();
+        ByteBuffer aggBuffer = aggBuffers.get(indexAndOffset[0]).get();


Hmm I feel that this fix to OffheapIncrementalIndex can be independent in its own separate issue and PR since it is a bug and would make it easier for tracking in the future.

a2l007 · 2020-12-09T23:17:24Z

benchmarks/src/test/java/org/apache/druid/benchmark/indexing/IncrementalIndexReadBenchmark.java

@@ -110,28 +119,28 @@ public void setup() throws IOException
    );

    incIndex = makeIncIndex();
+    gen.addToIndex(incIndex, rowsPerSegment);


I see there are other usages of gen.nextRow() that haven't been replaced. Is the plan to replace them in an follow up PR?

It wasn't planned, but I don't mind creating a follow-up PR for that or replacing everything in this PR.
What do you think is better?

Changing it in a follow-up PR sounds good to me.

OK. I'll publish a new PR for this once this PR is merged.

benchmarks/src/test/java/org/apache/druid/benchmark/indexing/IncrementalIndexReadBenchmark.java

liran-funaro · 2020-12-13T18:07:55Z

@a2l007 Thanks for your review. I made modifications accordingly.
Let me know if you think more changes should be made, or if we can proceed to merge it.

I have results for the benchmarks I changed, but they are not with the default parameters.
I'll run the benchmarks overnight and post the results here.

liran-funaro · 2020-12-15T08:45:41Z

@a2l007 The benchmark run results are available here: https://pastebin.pl/view/raw/8a5559c7

a2l007

LGTM apart from one minor comment.
Also I'm curious if you've noticed any improvement in the benchmark scores even though the benchmark related changes are mainly refactoring and parametrization?

a2l007 · 2020-12-15T19:53:17Z

benchmarks/src/test/java/org/apache/druid/benchmark/indexing/IncrementalIndexReadBenchmark.java

@@ -110,28 +119,28 @@ public void setup() throws IOException
    );

    incIndex = makeIncIndex();
+    gen.addToIndex(incIndex, rowsPerSegment);


Changing it in a follow-up PR sounds good to me.

a2l007 · 2020-12-15T20:40:24Z

benchmarks/src/test/java/org/apache/druid/benchmark/indexing/IndexPersistBenchmark.java

-  @Param({"none", "moderate", "high"})
-  private String rollupOpportunity;
+  @Param({"0", "1000", "10000"})
+  private int rollupOpportunity;


Sorry I missed this earlier, but I feel that we should retain the textual values for rollupOpportunity as that is more user-friendly when reading the benchmark results.

OK. I rolled back this change.

liran-funaro · 2020-12-16T10:39:44Z

Thanks, @a2l007. Please let me know what other modifications are required to approve this PR.

Regarding your question:

Also I'm curious if you've noticed any improvement in the benchmark scores even though the benchmark-related changes are mainly refactoring and parameterization?

First, it is much faster to run the benchmarks due to less redundant setup/teardown procedure calls (achieved via scopes).
However, for most of the benchmarks, I didn't notice any performance changes.

The exceptions are as follows:

IndexPersistBenchmark: this benchmark previously cleaned the temporary data folder inside the tested method, instead of in the teardown procedure. For large index sizes, it affected the benchmark result significantly.
With this modification, the results will be different (shorter times), but it will better reflect the "persist" performance.
IncrementalIndexRowTypeBenchmark: before the number of rows was not parametrized and it reported the time per single row insertion. Now it reports the total insertion time of all the rows, like the rest of the tests report. There is no option in JMH to set the @OperationsPerInvocation(MAX_ROWS) with respect to the parameter rowsPerSegment.
ScanBenchmark, SearchBenchmark: now using a fixed seed, so the results are reproducible but might be different than what was before with the random seed.

a2l007 · 2020-12-16T17:31:19Z

IndexPersistBenchmark: this benchmark previously cleaned the temporary data folder inside the tested method, instead of in the teardown procedure. For large index sizes, it affected the benchmark result significantly.
With this modification, the results will be different (shorter times), but it will better reflect the "persist" performance.

IncrementalIndexRowTypeBenchmark: before the number of rows was not parametrized and it reported the time per single row insertion. Now it reports the total insertion time of all the rows, like the rest of the tests report. There is no option in JMH to set the @OperationsPerInvocation(MAX_ROWS) with respect to the parameter rowsPerSegment.

ScanBenchmark, SearchBenchmark: now using a fixed seed, so the results are reproducible but might be different than what was before with the random seed.

I feel that this should be called out in the release notes so that users running benchmarks are aware of this change before upgrading. Could you please add a short description in the PR description mentioning the changes a user should expect running benchmarks before upgrading to this version. This would help the release manager add the blurb to the release notes.

a2l007

Thanks for the PR!

liran-funaro · 2020-12-16T18:12:59Z

@a2l007 I added a "User Experience Changes" section in the PR description.
Thanks!

- Reveal and fix a bug in OffheapIncrementalIndex

…g#format(java.lang.String,java.lang.Object[]) [Uses default locale]

liran-funaro · 2021-01-06T10:13:34Z

Can we proceed to merge this PR?

gianm · 2021-01-08T06:18:43Z

Can we proceed to merge this PR?

I haven't studied the patch, but the description makes sense to me (thanks for the detailed description), @a2l007 has reviewed it, and it seems pretty low risk (test/benchmark only) so I would say yes. Thanks for the contribution!

liran-funaro · 2021-01-08T14:51:12Z

Thanks!

* Remove redundant IncrementalIndex.Builder * Parametrize incremental index tests and benchmarks - Reveal and fix a bug in OffheapIncrementalIndex * Fix forbiddenapis error: Forbidden method invocation: java.lang.String#format(java.lang.String,java.lang.Object[]) [Uses default locale] * Fix Intellij errors: declared exception is never thrown * Add documentation and validate before closing objects on tearDown. * Add documentation to OffheapIncrementalIndexTestSpec * Doc corrections and minor changes. * Add logging for generated rows. * Refactor new tests/benchmarks. * Improve IncrementalIndexCreator documentation * Add required tests for DataGenerator * Revert "rollupOpportunity" to be a string

liran-funaro force-pushed the config-index-tests branch from 4a93dd0 to 288ee9e Compare November 18, 2020 13:58

liran-funaro changed the title ~~IncrementalIndex Tests and Benchmarks Refactor~~ IncrementalIndex Tests and Benchmarks Parametrization Nov 19, 2020

Eshcar reviewed Nov 29, 2020

View reviewed changes

liran-funaro force-pushed the config-index-tests branch from 6a05e86 to 559a9e8 Compare November 30, 2020 10:50

Eshcar approved these changes Dec 6, 2020

View reviewed changes

liran-funaro mentioned this pull request Dec 6, 2020

Move common configurations to TuningConfig #10478

Merged

8 tasks

a2l007 reviewed Dec 10, 2020

View reviewed changes

liran-funaro force-pushed the config-index-tests branch from 2f2bbc3 to 3a4d039 Compare December 13, 2020 17:23

liran-funaro force-pushed the config-index-tests branch from a7f195c to 7f6752f Compare December 13, 2020 21:50

a2l007 reviewed Dec 15, 2020

View reviewed changes

liran-funaro force-pushed the config-index-tests branch from 129abee to 334dbdd Compare December 16, 2020 10:16

a2l007 approved these changes Dec 16, 2020

View reviewed changes

a2l007 added Area - Testing Refactoring Release Notes labels Dec 16, 2020

liran-funaro added 10 commits December 22, 2020 12:37

Remove redundant IncrementalIndex.Builder

1df4fec

Parametrize incremental index tests and benchmarks

f4e0be9

- Reveal and fix a bug in OffheapIncrementalIndex

Fix forbiddenapis error: Forbidden method invocation: java.lang.Strin…

33a613f

…g#format(java.lang.String,java.lang.Object[]) [Uses default locale]

Fix Intellij errors: declared exception is never thrown

0d93694

Add documentation and validate before closing objects on tearDown.

07a6412

Add documentation to OffheapIncrementalIndexTestSpec

1ead19a

Doc corrections and minor changes.

1ffdf56

Add logging for generated rows.

a4ebea5

Refactor new tests/benchmarks.

43461f9

Improve IncrementalIndexCreator documentation

fc90379

liran-funaro added 2 commits December 22, 2020 12:37

Add required tests for DataGenerator

7818717

Revert "rollupOpportunity" to be a string

7527dde

liran-funaro force-pushed the config-index-tests branch from 334dbdd to 7527dde Compare December 22, 2020 15:02

gianm merged commit 08ab82f into apache:master Jan 8, 2021

gianm added a commit to gianm/druid that referenced this pull request Jan 8, 2021

Fix collision between apache#10689 and apache#10593.

fcbae78

gianm added a commit that referenced this pull request Jan 8, 2021

Fix collision between #10689 and #10593. (#10738)

6eef0e4

JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021

Fix collision between apache#10689 and apache#10593. (apache#10738)

f1d47e7

liran-funaro mentioned this pull request Jun 1, 2021

[Extension] oak-incremental-index: Low resource (RAM and CPU) incremental-index implementation using off-heap key/value map (OakMap) #10001

Closed

8 tasks

jihoonson added this to the 0.21.0 milestone Jul 15, 2021

IncrementalIndex Tests and Benchmarks Parametrization #10593

IncrementalIndex Tests and Benchmarks Parametrization #10593

Conversation

liran-funaro commented Nov 18, 2020 • edited

Description

Add test cases for each index type

Add benchmark cases for each index type

User Experience Changes

Runtime

Parametrization

Unified Benchmarks Behaviour

Eshcar left a 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

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

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

Choose a reason for hiding this comment

Eshcar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2l007 left a 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

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

liran-funaro commented Dec 13, 2020

liran-funaro commented Dec 15, 2020

a2l007 left a 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

liran-funaro commented Dec 16, 2020

a2l007 commented Dec 16, 2020

a2l007 left a comment

Choose a reason for hiding this comment

liran-funaro commented Dec 16, 2020

liran-funaro commented Jan 6, 2021

gianm commented Jan 8, 2021

liran-funaro commented Jan 8, 2021

liran-funaro commented Nov 18, 2020 •

edited