static to non-static conversion for methods in Index[Merger/Maker/IO] #1804

himanshug · 2015-10-05T18:14:27Z

so that appropriate dependencies can be injected specially ObjectMapper that can handle extensions as well without each having their own static Injector creation.

Release Notes update:
this PR removes the support for deprecated property druid.processing.bitmap and makes Concise be default.

xvrl · 2015-10-05T19:01:09Z

@himanshug nice! I've been waiting so long for that day. @cheddar you owe @himanshug a beer :)

xvrl · 2015-10-06T17:49:17Z

indexing-hadoop/src/main/java/io/druid/indexer/HadoopDruidIndexerConfig.java

@@ -84,6 +87,9 @@
  public static final Splitter tabSplitter = Splitter.on("\t");
  public static final Joiner tabJoiner = Joiner.on("\t");
  public static final ObjectMapper jsonMapper;


should we also update those constants above to uppercase to be consistent while we're at it?

sure, will do that

drcrallen · 2015-10-20T18:10:55Z

This is cool.

Will probably want to callout in release notes because this might break extensions (it will break some extensions I'm tinkering with).

himanshug · 2015-10-21T16:22:38Z

@drcrallen sure, we can call this out in release notes. I am putting a note on PR description too.

himanshug · 2015-10-23T19:37:12Z

@xvrl @drcrallen all comments have been taken care of, pls see as this PR is required to unblock #1728

gianm · 2015-10-31T20:40:48Z

indexing-hadoop/src/main/java/io/druid/indexer/HadoopDruidIndexerConfig.java

@@ -107,8 +113,11 @@ public void configure(Binder binder)
            new IndexingHadoopModule()
        )
    );
-    jsonMapper = injector.getInstance(ObjectMapper.class);
+    JSON_MAPPER = injector.getInstance(ObjectMapper.class);
    properties = injector.getInstance(Properties.class);


I think properties can be removed- it was only used for the "hack to get druid.processing.bitmap property passed down" which you've now got rid of

gianm · 2015-10-31T22:13:40Z

this PR looks good to me after figuring out what we want to do with druid.processing.bitmap.type - either remove it from the docs, or add support back in but mark it deprecated in the docs.

himanshug · 2015-10-31T23:47:14Z

@gian support for druid.processing.bitmap.type becomes complicated inside IndexSpec no arg constructor once we lose static properties object inside IndexIO also it needs hacks to support it inside hadoop indexing as well . Code said it was deprecated (dint look at the doc at the time) and hence removed it to keep things simple. Since this is going inside a new major release, I believe it is OK.

gianm · 2015-11-01T00:21:48Z

@himanshug ok sounds fair

gianm · 2015-11-01T00:21:52Z

👍 looks good to me

drcrallen · 2015-11-02T21:03:33Z

👍

xvrl · 2015-11-02T21:54:01Z

indexing-service/src/main/java/io/druid/indexing/common/task/IndexTask.java

-      @JsonProperty("context") Map<String, Object> context
+      @JsonProperty("context") Map<String, Object> context,
+      @JacksonInject IndexMerger indexMerger,
+      @JacksonInject IndexIO indexIO


Injecting IndexIO and IndexMerger might be a problem for external libraries relying on those classes purely for serialization and de-serialization, such as tranquility. I wonder if it would be possible to separate the parts necessary for serde from the parts needed to run the task.

Could put them in the TaskToolbox instead.

the same concern also applies to all the other tasks and classes that, until now, did not rely on jackson injection.

we should also add some ser-de tests for tasks, I don't think we have any.

there's a lot in the TaskSerdeTest class

Fwiw I think tranquility will probably be able to handle these, as long as its injector has the ability to make these classes (which if it doesn't, could be added). Putting them in the TaskToolbox is cool too if we want to go that way.

@gianm my bad, missed the task serde test. In general I think it would be nice to move away from jackson injecting if we ever want to make those classes available in a proper API or client library.

@xvrl @gian if we put it inside TaskToolbox then TaskToolbox needs to be injected , I don't think we can avoid injection.
libraries will probably need to setup dummy objects for injection for all these.

@himanshug the toolbox shouldn't need to be injected just for serialization, as it's only needed if you're calling run. That wouldn't happen if the *Task class is just being used by something that wants to generate a request to submit to the overlord (like tranquility does). Right?

I am ok with doing it either way though. Tranquility already has to set up an ObjectMapper with a bunch of Druid modules (for stuff like AggregatorFactories and Firehoses) and so needing this other stuff too wouldn't be much worse.

@xvrl @gianm ok removed them from individual task constructors, added to TaskToolbox instead and using from same.
hopefully this will reduce some burden on tranquility side.

xvrl · 2015-11-02T22:05:38Z

@himanshug I had marked default bitmap config deprecated mainly because it is possible to specify it in the index spec ever since we added compression, whereas before that property was the only way to specify it. It might still be nice to be able to specify a default, but I'm also not wedded to it if you think it's too cumbersome to maintain. A more general way to specify indexing default might be nice in the future though.

xvrl · 2015-11-02T22:37:49Z

other than my concerns about jackson injection, the rest looks fine to me.

appropriate ObjectMapper injected instead of creating one statically

himanshug · 2015-11-03T05:25:56Z

@xvrl all comments addressed

gianm · 2015-11-03T17:31:24Z

changes looks good, 👍

xvrl · 2015-11-03T19:16:09Z

👍

static to non-static conversion for methods in Index[Merger/Maker/IO]

himanshug force-pushed the objectify_index_creators branch from 6f6d465 to 3b20e2d Compare October 5, 2015 18:21

himanshug added this to the 0.9.0 milestone Oct 5, 2015

himanshug mentioned this pull request Oct 5, 2015

Store AggregatorFactory[] in segment metadata #1728

Merged

xvrl reviewed Oct 6, 2015
View reviewed changes

himanshug force-pushed the objectify_index_creators branch 3 times, most recently from 28837df to 2189b2b Compare October 21, 2015 20:04

himanshug mentioned this pull request Oct 30, 2015

Cleanup the Druid codebase #1895

Merged

gianm reviewed Oct 31, 2015
View reviewed changes

himanshug force-pushed the objectify_index_creators branch from 2189b2b to 02ca44e Compare October 31, 2015 23:40

xvrl reviewed Nov 2, 2015
View reviewed changes

himanshug force-pushed the objectify_index_creators branch from 02ca44e to 84f7d8d Compare November 3, 2015 05:24

himanshug added 2 commits November 2, 2015 23:24

make methods in Index[Merger,Maker,IO] non-static so that they can have

8b67417

appropriate ObjectMapper injected instead of creating one statically

making static final variables in HadoopDruidIndexerConfig upper case

84f7d8d

gianm closed this Nov 3, 2015

gianm reopened this Nov 3, 2015

fjy added a commit that referenced this pull request Nov 3, 2015

Merge pull request #1804 from himanshug/objectify_index_creators

cec09a9

static to non-static conversion for methods in Index[Merger/Maker/IO]

fjy merged commit cec09a9 into apache:master Nov 3, 2015

himanshug added the Incompatible label Nov 10, 2015

himanshug deleted the objectify_index_creators branch December 5, 2015 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

static to non-static conversion for methods in Index[Merger/Maker/IO] #1804

static to non-static conversion for methods in Index[Merger/Maker/IO] #1804

himanshug commented Oct 5, 2015

xvrl commented Oct 5, 2015

xvrl Oct 6, 2015

himanshug Oct 6, 2015

himanshug Oct 21, 2015

drcrallen commented Oct 20, 2015

himanshug commented Oct 21, 2015

himanshug commented Oct 23, 2015

gianm Oct 31, 2015

himanshug Oct 31, 2015

gianm commented Oct 31, 2015

himanshug commented Oct 31, 2015

gianm commented Nov 1, 2015

gianm commented Nov 1, 2015

drcrallen commented Nov 2, 2015

xvrl Nov 2, 2015

gianm Nov 2, 2015

xvrl Nov 2, 2015

xvrl Nov 2, 2015

gianm Nov 2, 2015

gianm Nov 2, 2015

xvrl Nov 2, 2015

himanshug Nov 2, 2015

gianm Nov 3, 2015

himanshug Nov 3, 2015

xvrl commented Nov 2, 2015

xvrl commented Nov 2, 2015

himanshug commented Nov 3, 2015

gianm commented Nov 3, 2015

xvrl commented Nov 3, 2015

static to non-static conversion for methods in Index[Merger/Maker/IO] #1804

static to non-static conversion for methods in Index[Merger/Maker/IO] #1804

Conversation

himanshug commented Oct 5, 2015

xvrl commented Oct 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Oct 20, 2015

himanshug commented Oct 21, 2015

himanshug commented Oct 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm commented Oct 31, 2015

himanshug commented Oct 31, 2015

gianm commented Nov 1, 2015

gianm commented Nov 1, 2015

drcrallen commented Nov 2, 2015

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

xvrl commented Nov 2, 2015

xvrl commented Nov 2, 2015

himanshug commented Nov 3, 2015

gianm commented Nov 3, 2015

xvrl commented Nov 3, 2015