Configurable Index Type by liran-funaro · Pull Request #10335 · apache/druid

liran-funaro · 2020-08-31T08:02:37Z

See #10321 for details.

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.

Key changed/added classes in this PR

AppendableIndexBuilder
AppendableIndexSpec
TuningConfig
AppenderatorConfig

Eshcar

This PR is bloated with similar changes due to refactoring
specifically the change from buildOnHeap() to build() - it affects more than 100 files
maybe there is a way to avoid it or have it scoped in a separate PR so that it would be easier to see and review exactly the changes that add the configuration capability

I added some comments suggesting how to further reduce the number of unnecessary changes (like null params, etc.)

Other than that looks good to me

a2l007

+1 for the extensible design that makes it easier to use new incremental index types. It would be useful to have integration tests for this which can test the appendableIndexSpec property within tuningConfig.

liran-funaro · 2020-10-08T13:22:32Z

Thanks, @a2l007
I addressed all your comments in the codes, or with follow-up questions.
Regarding the integration tests, I added tests to all the TuningConfig tests that validate the AppendableIndexSpec within TuningConfig.
I think it is sufficient. Do you agree?

a2l007

Thanks, I've left a couple more comments.
Since we're introducing a new property within tuningConfig, I feel it would be useful to have a couple of integration tests with this property in the ingestion spec. However, I would be okay without adding them if the remaining community reviewers are fine without it.

Could you please fix the CI failures as well? Looks like there is coverage failure.

liran-funaro · 2020-10-08T16:47:41Z

Could you please fix the CI failures as well? Looks like there is coverage failure.

The coverage gap comes from ParallelIndexTuningConfig.equals() and it was there before this PR. I just added another comparison.
It covers 10/20 branches, but it will require writing 10 additional unique tests just to cover it.

liran-funaro · 2020-10-15T13:34:28Z

@a2l007 I addressed all your comments. Is there anything else I need to do to move this PR forward?

a2l007

Sorry for the delay. Just one additional comment, which should help with fixing the travis failures.

a2l007

LGTM. Thanks!

liran-funaro · 2020-10-15T20:15:21Z

@a2l007 Thanks!

liran-funaro · 2020-10-20T14:46:00Z

@jihoonson Do you have any more comments or can we proceed to merge this PR?

jihoonson

Hi @liran-funaro, I haven't reviewed line by line, but left some comments especially for documentation.

jihoonson · 2020-10-20T20:40:39Z

+|-----|-----------|-------|
+|type|Each in-memory index has its own tuning type code. You must specify the type code that matches your in-memory index. Common options are `onheap`, and `offheap`.|`onheap`|
+
+Beyond these properties, each in-memory index has its own specific tuning properties.


Do you think if users want to change this config ever for now? I remember @gianm mentioned that the offheap incremental index has some performance issue, which is why we don't use it for indexing. If you think this knob is useful for users, please add more details how each index type is different and when it's recommended to use what. Otherwise, I would suggest not documenting this know at least for now.

Actually, by @a2l007 suggestion, I deferred the support for offheap ingestion to a different PR.
So this documentation should not include the offheap option anyway.
I'll remove this documentation for now.

liran-funaro · 2020-10-21T11:17:19Z

@jihoonson Thanks. I addressed all your comments.
Please let me know if you see any other required modifications, or that we could proceed to merge this PR.

jihoonson

LGTM. +1 after CI. Thanks @liran-funaro!

liran-funaro · 2020-10-22T22:13:51Z

Passed CI.
Thanks @jihoonson and @a2l007

liran-funaro · 2020-10-23T14:19:46Z

@jihoonson Do we need more approval before merge?

jihoonson · 2020-10-23T16:34:46Z

@liran-funaro we usually wait for some time before merging a PR for other committers if there is anyone who wants to review as well. I will merge this PR today unless there is someone else who has more comments.

jihoonson · 2020-10-24T01:34:35Z

@liran-funaro merged. Thanks!

* Introduce a Configurable Index Type * Change to @UnstableApi * Add AppendableIndexSpecTest * Update doc * Add spelling exception * Add tests coverage * Revert some of the changes to reduce diff * Minor fixes * Update getMaxBytesInMemoryOrDefault() comment * Fix typo, remove redundant interface * Remove off-heap spec (postponed to a later PR) * Add javadocs to AppendableIndexSpec * Describe testCreateTask() * Add tests for AppendableIndexSpec within TuningConfig * Modify hashCode() to conform with equals() * Add comment where building incremental-index * Add "EqualsVerifier" tests * Revert some of the API back to AppenderatorConfig * Don't use multi-line comments * Remove knob documentation (deferred)

exherb · 2022-06-13T07:27:46Z

Looks this feature has been removed? I try to index kafka stream with off heap but got ignored.

liran-funaro mentioned this pull request Aug 31, 2020

[Proposal] Design for a Configurable Index Type #10321

Closed

liran-funaro force-pushed the config-index branch 3 times, most recently from 4cfcbed to b552f94 Compare September 1, 2020 15:05

liran-funaro force-pushed the config-index branch from cef20bf to 7885ba0 Compare September 13, 2020 07:15

liran-funaro changed the title ~~[WIP] Design for a Configurable Index Type~~ WIP Design for a Configurable Index Type Sep 13, 2020

liran-funaro changed the title ~~WIP Design for a Configurable Index Type~~ [WIP] Design for a Configurable Index Type Sep 13, 2020

liran-funaro changed the title ~~[WIP] Design for a Configurable Index Type~~ Design for a Configurable Index Type Sep 21, 2020

liran-funaro changed the title ~~Design for a Configurable Index Type~~ Configurable Index Type Sep 21, 2020

liran-funaro force-pushed the config-index branch from 83c781f to 90fc315 Compare September 21, 2020 10:15

liran-funaro force-pushed the config-index branch 3 times, most recently from 85a422c to 7e62549 Compare October 4, 2020 14:31

liran-funaro added 6 commits October 4, 2020 17:34

Introduce a Configurable Index Type

928f929

Change to @UnstableApi

a9fbe77

Add AppendableIndexSpecTest

e089d09

Update doc

1e5d8ba

Add spelling exception

dea7e23

Add tests coverage

efe35fd

liran-funaro force-pushed the config-index branch from 7e62549 to efe35fd Compare October 4, 2020 14:34

Eshcar approved these changes Oct 5, 2020

View reviewed changes

liran-funaro mentioned this pull request Oct 5, 2020

Move common configurations to TuningConfig #10478

Merged

8 tasks

a2l007 reviewed Oct 5, 2020

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/segment/incremental/AppendableIndexBuilder.java

Revert some of the changes to reduce diff

452b196

liran-funaro force-pushed the config-index branch from 5d791c1 to 452b196 Compare October 6, 2020 14:09

a2l007 requested changes Oct 7, 2020

View reviewed changes

a2l007 added Area - Batch Ingestion Area - Streaming Ingestion Design Review labels Oct 7, 2020

Minor fixes

56ed0b5

liran-funaro requested a review from a2l007 October 8, 2020 13:24

a2l007 reviewed Oct 8, 2020

View reviewed changes

Comment thread indexing-hadoop/src/main/java/org/apache/druid/indexer/HadoopTuningConfig.java

Comment thread processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java

Comment thread server/src/main/java/org/apache/druid/segment/indexing/TuningConfig.java Outdated

Add comment where building incremental-index

829b86f

liran-funaro mentioned this pull request Oct 8, 2020

Remove redundant IncrementalIndex.Builder #10494

Closed

a2l007 reviewed Oct 15, 2020

View reviewed changes

Comment thread .../main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskTuningConfig.java

Add "EqualsVerifier" tests

afac4d8

a2l007 approved these changes Oct 15, 2020

View reviewed changes

Revert some of the API back to AppenderatorConfig

5c8c18f

jihoonson reviewed Oct 20, 2020

View reviewed changes

liran-funaro added 2 commits October 21, 2020 14:05

Don't use multi-line comments

b04ae43

Remove knob documentation (deferred)

7de9b49

jihoonson approved these changes Oct 22, 2020

View reviewed changes

jihoonson merged commit f3a2903 into apache:master Oct 24, 2020

liran-funaro mentioned this pull request Nov 18, 2020

IncrementalIndex Tests and Benchmarks Parametrization #10593

Merged

8 tasks

jihoonson added this to the 0.21.0 milestone Jan 4, 2021

jihoonson mentioned this pull request Jan 13, 2021

[Draft] 0.21.0 Release Notes #10752

Closed

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

Conversation

liran-funaro commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key changed/added classes in this PR

Uh oh!

Eshcar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a2l007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liran-funaro commented Oct 8, 2020

Uh oh!

a2l007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liran-funaro commented Oct 8, 2020

Uh oh!

liran-funaro commented Oct 15, 2020

Uh oh!

a2l007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a2l007 left a comment

Choose a reason for hiding this comment

Uh oh!

liran-funaro commented Oct 15, 2020

Uh oh!

liran-funaro commented Oct 20, 2020

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jihoonson Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

liran-funaro Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liran-funaro commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

liran-funaro commented Oct 22, 2020

Uh oh!

liran-funaro commented Oct 23, 2020

Uh oh!

jihoonson commented Oct 23, 2020

Uh oh!

jihoonson commented Oct 24, 2020

liran-funaro commented Aug 31, 2020 •

edited

Loading

liran-funaro Oct 21, 2020 •

edited

Loading

liran-funaro commented Oct 21, 2020 •

edited

Loading