ability to build and load custom segment in realtime node by kaijianding · Pull Request #4448 · apache/druid

kaijianding · 2017-06-22T18:53:27Z

It's a follow up of #2965 #3901

introduce sinkFactory to use custom sink to handle events, like not using onHeapIncrementalIndex to hold events, but still using storageAdapter interface when query
introduce customIndexMerger to build custom segment when persist and merge
can load custom segment when bootstrapSinksFromDisk and after persist success

leventov · 2017-06-22T19:57:29Z

@kaijianding you are doing a lot of PRs, could you also review other people's PRs? And then become a committer?

kaijianding · 2017-06-22T20:08:05Z

Sure, will review other's PRs and glad to hear that I'm close to being a committer @leventov

leventov · 2017-06-22T20:17:48Z

@kaijianding I'm not sure about closeness and it's not for me to decide because I'm not a PMC member, but making reviews is essential because it's a painful aspect in the project that not enough reviews are done. When you are making a lot of PRs you demand reviews but not "contribute them back".

KurtYoung · 2017-06-23T00:55:25Z

I will try to review this. Sorry for not having much time to do the review.

gianm · 2017-06-23T20:05:39Z

cc @cheddar @pjain1 who may be able to help review as well due to previous work in the area.

pjain1 · 2017-06-23T22:17:16Z

I can give it a try next week

KurtYoung · 2017-06-28T08:40:29Z

What is the differences between loadSegment().asQueryableIndex() and just loadIndex()?
I noticed there still exists plenty usage of loadIndex(), should they also be changed?

every place want to load index from file should call loadSegment instead of loadIndex. inside loadSegment, it will use the customized segmentizerFactory to map the file to index.
Most usages of loadIndex() are in tests and benchmarks, so I don't modify them.
I will double check the rest usages to make sure every place calling loadIndex is replaced by loadSegment()

If loadSegment is the only correct way to load a segment, then i would suggest you to delete the `loadIndex' method.

I rewrite the indexIO.loadIndex() to use segmentizerFactory to load index from file, then there is no need to modify all the place calling indexIO.loadIndex() now

KurtYoung · 2017-06-28T08:43:40Z

Do we need this?

yes, otherwise the test will fail.

public AlertBuilder makeAlert(Throwable t, String message, Object... objects) { if (emitter == null) { final String errorMessage = String.format( "Emitter not initialized! Cannot alert. Please make sure to call %s.registerEmitter()", this.getClass() );

the emitter is registered in EmittingLogger.registerEmitter

Can you double check this? I commented this line and the test passed. BTW, why this test will not fail before you added this, and why the test case would enter the logic about making any alerts?

after rewrite indexIO.loadIndex(), there is no need to modify this anymore

KurtYoung · 2017-06-28T08:46:08Z

Do we need this?

yes, the customIndexMerger may need to use Metadata, like in my customIndexMerger implement.

Can we change this until we see a real customized index merger?

KurtYoung · 2017-06-28T08:46:18Z

Do we need this?

my implement want to sub class SmooshedFileMapper, so need it can be extended.

@kaijianding SmooshedFileMapper is not a part of public api, and could be changed anytime so that your subclass will break. Worse, the behaviour may break, however without compilation errors, so you won't notice it. So if you want something similar to SmooshedFileMapper in your custom SegmentizerFactory, I strongly suggest to copy SmooshedFileMapper entirely into your codebase and tailor it for your needs.

I want to use V9IndexLoader.load(SmooshedFileMapper smooshedFiles, File inDir, ObjectMapper mapper), this method requires SmooshedFileMapper.
It's fine SmooshedFileMapper can change behaviors at any time, I extend SmooshedFileMapper in my extension and will have unit test to ensure everything is working properly.

KurtYoung · 2017-06-28T08:49:14Z

Does this related to your proposed change?

yes, this is used to load persisted segment in realtime node

change it to loadIndex()

KurtYoung · 2017-06-28T08:49:59Z

Looks like this is some kind of performance improvement?

this is some cleanup to StringDimensionMergerV9.java, in this part, only dimValuesList is needed, but more info is required(the adapters) than needed.

So this is unrelated to this PR, right? Can we separate this to another PR?

KurtYoung

Looks like this PR contains 3 different changes:

make SegmentizerFactory works in some cases
introduce sinkFactory and customIndexMeger
some performance improvement

I think these 3 things are completely independent, can you split this PR into three?

kaijianding · 2017-06-28T11:18:58Z

The purpose of this PR is to provide the ability to build and load customized segment. SinkFactory and customizeIndexMerger are used to build customized segment, and the indexIo.loadSegment() is to load the customized segment So I think the 3 modifications are better in one PR. @KurtYoung

KurtYoung · 2017-06-28T12:22:55Z

Make sense except the third point. Overall it looks good to me. @leventov Do you have any design concerns?

leventov · 2017-06-30T06:08:08Z

  @Deprecated
  private final long handoffConditionTimeout;
  private final boolean resetOffsetAutomatically;
+  private final SinkFactory sinkFactory;


Please extract an umbrella abstraction like "SegmentStrategy" with createSinkFactory() and createIndexMerger() methods, and inject/serialize/deserialize only it, here and in other places.

leventov · 2017-06-30T06:09:40Z

    }
-    return indexSpec != null ? indexSpec.equals(that.indexSpec) : that.indexSpec == null;
-
+    if (indexSpec != null ? !indexSpec.equals(that.indexSpec) : that.indexSpec != null) {


Refactor to use Objects.equals()

it's generated by Intellij Idea, but sure, will modify it.

You can generate the forms that I suggested with intelliJ as well, need to choose another option in generation dialog. Once you choose it, it will be the default later.

leventov · 2017-06-30T06:09:54Z

    result = 31 * result + (reportParseExceptions ? 1 : 0);
    result = 31 * result + (int) (handoffConditionTimeout ^ (handoffConditionTimeout >>> 32));
    result = 31 * result + (resetOffsetAutomatically ? 1 : 0);
+    result = 31 * result + (sinkFactory != null ? sinkFactory.hashCode() : 0);


Refactor to use Objects.hash()

leventov · 2017-06-30T06:11:09Z

    // Set of spilled segments. Will be merged at the end.
    final Set<File> spilled = Sets.newHashSet();

+    // IndexMerger implementation.


Not useful comment

leventov · 2017-06-30T06:12:21Z

    final Set<File> spilled = Sets.newHashSet();

+    // IndexMerger implementation.
+    final IndexMerger theIndexMerger = config.getCustomIndexMerger() != null


Regarding null handling - doesn't Jackson handle it up front, when you specify defaultImpl=... on the IndexMerger interface (or SegmentStrategy for that matter, if you apply suggestion above)?

usually the default implement is provided in the class context, like indexMergerV9 here. Also the TaskToolbox has the default implement, like toolbox.getIndexMergerV9().
So I think it's better to regard the old code here to get the default implement.

leventov · 2017-06-30T06:24:49Z

  }

-  static class V9IndexLoader implements IndexLoader
+  public static class V9IndexLoader implements IndexLoader


Why public?

see comments below

leventov · 2017-06-30T06:27:04Z

+  {
+    return getSegmentizerFactory(inDir).loadIndex(inDir);
+  }
+


Readers and users will never know the difference between loadIndex() and loadIndexDirectly(). Maybe move loadIndexDirectly() to MMappedQueryableSegmentizerFactory. Along with some other methods.

ok, but many related code should be moved to.

leventov · 2017-06-30T06:28:21Z

+@JsonSubTypes(value = {
+    @JsonSubTypes.Type(name = "v9", value = IndexMergerV9.class)
+})
 @ImplementedBy(IndexMergerV9.class)


@ImplementedBy is likely not needed since Json defaultImpl is added.

@ImplementedBy is guice thing, I'm not sure it can work properly after removing it if it is not a json case

leventov · 2017-06-30T06:30:41Z

    setupEncodedValueWriter();
  }

+  protected List<Indexed<String>> toDimValuesList(List<IndexableAdapter> adapters) throws IOException


As far as I can understand you changed this class in order to be able to extend it in your code. See comment to SmooshedFileMapper, don't do this. Copy class entirely into your codebase.

leventov · 2017-06-30T06:32:05Z

@@ -174,7 +174,7 @@ public static final AggregationTestHelper createSelectQueryAggregationTestHelper
        new InjectableValues.Std().addValue(


Formatting

new InjectableValues.Std() .addValue(SelectQueryConfig.class, new SelectQueryConfig(true)) .addValue(IndexIO.class, TestHelper.getTestIndexIO())

gianm · 2017-06-30T06:48:37Z

Just a drive by comment on the public api stuff. I believe none of the segment-type extension points are public apis at this point (see http://druid.io/docs/latest/development/modules.html) and that's because we're not yet entirely sure what custom segment extensions should look like and what support they might need from core druid.

IMO, if @kaijianding is ok with this then it's fine for his custom extensions to use internal apis. It's part of the learning process of what should and shouldn't be public for these kinds of extensions. At some point in the future, we might have a better idea of what should be public, and then we can make those apis public.

kaijianding · 2017-06-30T07:04:33Z

I don't build custom segment from the very beginning but do some wrap on top of the old codes, like MetaData, Smoosh, V9 indexMerger and V9 loader, the sink and onHeapIncreamentalIndex.

I modified the structure of the final smooshed file, but I didn't modify the v9 format itself. Then I need to access the metadata.drd and use v9 loader to load index from the smooshed file from position P1 to Pn. That's the reason I want MetaData and IndexLoader to be public, thus I can use it in my extension to load the custom segment from file. @gianm @leventov

leventov · 2017-06-30T07:11:30Z

@kaijianding I'm ok with just making (or keeping) classes and constructors public, but I'm not OK with adding properties and fields which are unused in the core codebase and protected methods to classes that have no subclasses in the core codebase. It increases complexity of the core for nothing. And it's not a step towards modular design, the ultimate goal of this PR as well.

gianm · 2017-06-30T07:22:21Z

Are there a lot of properties and fields in the patch that don't need to be protected or even exist at all? (Sorry, I haven't had a chance to read it yet). I'm hoping we can find a balance between keeping the core clean, and making the extension possible.

kaijianding · 2017-06-30T07:39:26Z

Fine, will restore some changes and modify them when I provide an actually custom implement @leventov

stale · 2019-02-28T05:15:10Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2019-03-07T06:09:27Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

kaijianding force-pushed the customSegment branch 2 times, most recently from 618ab1a to 7080bbb Compare June 22, 2017 19:16

kaijianding force-pushed the customSegment branch from 7080bbb to 7b78f82 Compare June 22, 2017 19:59

kaijianding force-pushed the customSegment branch from 7b78f82 to cfd979c Compare June 22, 2017 20:52

KurtYoung self-assigned this Jun 23, 2017

kaijianding force-pushed the customSegment branch 19 times, most recently from b27a368 to d31abb7 Compare June 27, 2017 06:01

leventov self-assigned this Jun 28, 2017

KurtYoung reviewed Jun 28, 2017

View reviewed changes

kaijianding force-pushed the customSegment branch 4 times, most recently from 7053cf0 to 4f82eb8 Compare June 28, 2017 18:47

kaijianding added 3 commits June 29, 2017 03:40

introduce SinkFactory in tunningConfig

34e3e57

introduce customIndexMerger in tunningConfig

f8896c3

ability to load customized segment format

e3256fc

kaijianding force-pushed the customSegment branch from 4f82eb8 to e3256fc Compare June 28, 2017 19:42

leventov requested changes Jun 30, 2017

View reviewed changes

clambertus unassigned leventov and KurtYoung Jul 6, 2018

stale Bot added the stale label Feb 28, 2019

stale Bot closed this Mar 7, 2019

		@@ -174,7 +174,7 @@ public static final AggregationTestHelper createSelectQueryAggregationTestHelper
		new InjectableValues.Std().addValue(

Conversation

kaijianding commented Jun 22, 2017

Uh oh!

leventov commented Jun 22, 2017

Uh oh!

kaijianding commented Jun 22, 2017

Uh oh!

leventov commented Jun 22, 2017

Uh oh!

KurtYoung commented Jun 23, 2017

Uh oh!

gianm commented Jun 23, 2017

Uh oh!

pjain1 commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KurtYoung Jun 28, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KurtYoung left a comment

Choose a reason for hiding this comment

Uh oh!

kaijianding commented Jun 28, 2017

Uh oh!

KurtYoung commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

KurtYoung Jun 28, 2017 •

edited

Loading

leventov Jun 30, 2017 •

edited

Loading

kaijianding Jun 30, 2017 •

edited

Loading