Add @ExtensionPoint and @PublicApi annotations. by gianm · Pull Request #4433 · apache/druid

gianm · 2017-06-21T01:04:42Z

This is an attempt to clarify questions about what is and is not a public API (see #4131 (comment), #4394 (comment), #4254 (comment)). It adds two new annotations: @ExtensionPoint which signifies something you can subclass, and @PublicApi which signifies something you're not meant to subclass, but that you can use for implementation. The idea is that if an extension author sticks to public APIs then their extension will keep working for at least an entire major Druid line.

Extension authors are of course free to use non-public APIs, but their extensions might break randomly.

Feedback on the specific list of annotated and non-annotated items is welcome.

Some notes:

The list of public apis is somewhat subjective, but I did my best guess based on the current contents of http://druid.io/docs/latest/development/modules.html and based on recent discussions about what is and isn't public. In general I erred on the side of annotating fewer rather than more entities, since I figured that annotating too many entities would make too much friction for dev of core Druid.
Speaking of subjectivity: relating to the last comment in the list above (Add support for headers and skipping thereof for CSV and TSV #4254 (comment)) I actually think CSVParseSpec shouldn't be considered a public api, since it's not really useful for extensions. So, I didn't annotate it. Tranquility does use it, but Tranquility really blatantly uses a ton of private Druid APIs. It should really have no expectation of being able to run with any Druid release other than the one it was built with.
Nothing in java-util is annotated since the annotations are in druid-api, and java-util doesn't depend on druid-api. We could deal with this by either saying "everything in java-util is public", or alternatively, creating a new package druid-annotations that java-util and druid-api both depend on.
I'm not sure I got the annotations for the query metrics stuff right (see QueryMetrics: abstraction layer of query metrics emitting (part of #3798) #3954). @leventov could you double check if I captured your intent? What I did was annotate GenericQueryMetricsFactory as public (since AFAIK any extension query would need to call methods on it) but nothing else in the query metrics world.

leventov · 2017-06-21T01:22:51Z

Is druid-api module still needed?

gianm · 2017-06-21T01:25:46Z

Maybe not. The purpose was to put everything that was a public API in one package, but it turns out that there are public APIs in other packages too.

gianm · 2017-06-21T15:18:30Z

I cleaned up the wording a bit, to clarify that PublicApis can have new non-default methods added to an interface and this is not considered a breakage (because they are not meant to be subclassed).

fjy · 2017-06-23T21:29:55Z

👍

leventov · 2017-07-03T23:33:23Z

+ *
+ * @see PublicApi
+ */
+@Target({ElementType.TYPE, ElementType.FIELD, ElementType.METHOD, ElementType.CONSTRUCTOR})


How could a field, method or constructor be an extension point?

It can't, this is a copy/paste error. I'll remove those.

leventov · 2017-07-03T23:36:56Z

 /**
 */
+@PublicApi
 public class Runnables


Such utility classes are not generally in API, they are in druid-common

This one is probably here since it's used by FileIteratingFirehose which is in druid-api.

leventov · 2017-07-03T23:39:23Z

 /**
 */
+@PublicApi
 public class Queries


Should it really be public API?

It's utility methods that are use in the implementations of most built-in queries and post-aggregators. If it's not a public api, then people that implement their own extension based on the built-in code (which is probably common) will accidentally use internal apis.

There's a judgement call here: extension implementors could always copy/paste the code from Queries rather than linking to it directly, and there is cost to the core project for having too many public apis. But I think that in this case it's worth it.

leventov · 2017-07-03T23:41:36Z

  @Nullable
  public ColumnCapabilities getColumnCapabilities(String column);

-  public Map<String, DimensionHandler> getDimensionHandlers();


Why remove this?

It was unused, and I didn't want it to be part of the public API of StorageAdapter.

leventov · 2017-07-03T23:42:19Z

    this.columnSize = columnSize;
  }

+  @PublicApi


Why it needs to be public API?

Since it's commonly used in implementations of ComplexMetricsSerde, which is an extension point.

gianm · 2017-07-05T15:34:55Z

Pushed a new commit to resolve conflicts and address #4433 (comment).

gianm · 2017-07-07T22:38:09Z

Thanks for your reviews @fjy and @leventov. I'd like to get a few more eyes on this just to see what other people think. Let's keep it open for a bit and bring it up in a dev sync too.

drcrallen · 2017-07-25T17:58:46Z

@gianm I have heard from @cheddar before that the json is the api spec rather than the java code itself per-se. Since there is a mix of things which feed into jackson and things which don't feed into jackson that have the same annotation, would it make sense to have a special annotation for things which are part of the JSON api, rather than the Java api?

himanshug · 2017-07-25T18:07:51Z

 import java.util.List;

 /**
+ * Not an {@code ExtensionPoint} since extension parsers are meant to extend {@code InputRowParser}.


given that InputRowParser is an extension point, this needs to be an extension point or else how would they implement InputRowParser.getParseSpec() ?

That's a good point…

I guess ParseSpec should be an extension point? And that would mean TimestampSpec and DImensionsSpec should be public apis.

I'll change it.

Added that in the most recent commit.

himanshug · 2017-07-25T18:09:15Z

 /**
 */
+@PublicApi
 public class StringInputRowParser implements ByteBufferInputRowParser


do we expect extensions to directly reference StringInputRowParser ?

I was thinking it would be useful for people that want to implement extension firehoses that parse strings. Otherwise they'll need to duplicate a lot of logic.

Maybe nobody really uses it though and we can take out the annotation. Hmm, ok I'll take it out.

himanshug · 2017-07-25T18:11:59Z


 /**
 */
+@PublicApi


actually its more of an utility class and shouldn't necessarily be directly used in custom user extension (it can be used in out-of-the-box extensions) and those should really just use MapBinder.xxx .

I was thinking it would be nice to make a utility like this public. I don't feel super strongly though.

gianm · 2017-07-25T18:13:01Z

@drcrallen,

@gianm I have heard from @cheddar before that the json is the api spec rather than the java code itself per-se. Since there is a mix of things which feed into jackson and things which don't feed into jackson that have the same annotation, would it make sense to have a special annotation for things which are part of the JSON api, rather than the Java api?

For this I think the attitude has been (and can continue to be) that a JSON API is stable if, and only if, it is documented. So I don't think an annotation is necessary.

gianm · 2017-07-27T00:47:13Z

It was brought up on the dev sync that it would be desirable to merge java-util into druid-common, which would solve the problem of "java-util can't see the annotations". That would be a big patch though and I think I'd like to do this patch first, for everything other than java-util. And then do another one for merging in java-util to druid-common.

himanshug · 2017-07-27T19:04:39Z

 */
-public interface
-    InputRow extends Row
+@PublicApi


shouldn't this be an extension point instead ? InputRowParser is an extension point and parse(..) should be free to return a concrete implementation that it wants.

I was thinking they would all use MapBasedRow, since the current parsers do that, rather than implement their own Row subclasses. We can make it an extension point if you think that makes more sense though.

hmmm, so I took a brief look at avro and orc parsers which create MapBasedInputRow and it looks like it would be better to create an impl of InputRow instead .

to use MapBasedInputRow..

avro parser makes GenericRecord look like a Map here ( https://github.com/druid-io/druid/blob/master/extensions-core/avro-extensions/src/main/java/io/druid/data/input/avro/GenericRecordAsMap.java ) and most methods have "unimplemented exceptions" , would probably be better to just make GenericRecord look like an InputRow.

orc parser converts its ord record into a map here ( https://github.com/druid-io/druid/blob/master/extensions-contrib/orc-extensions/src/main/java/io/druid/data/input/orc/OrcHadoopInputRowParser.java#L78 ) which would be inefficient if your input record had alot of columns that were not being indexed into druid. again it would be better to make orc record look like an InputRow instead.

with that information, what do you think ?

Hmm, thanks for looking into that. In that case I think it makes sense to make this an extension point. I'll change it.

himanshug · 2017-07-27T19:07:47Z


 /**
 */
+@PublicApi


is this public api so that users can use this class in their java clients to construct druid queries ?

It's because ParseSpec is an extension point, and it returns DimensionsSpec, which is constructed using DimensionSchema. So implementors of custom ParseSpecs might need to use this class.

himanshug · 2017-07-27T19:29:09Z

+import io.druid.guice.annotations.PublicApi;
 import org.joda.time.Interval;

+@PublicApi


QueryToolChest.filterSegments(..) returns an impl of this , so this needs to be an extension point.

It's supposed to return the same objects that it was given (but possibly filtered). So it doesn't need to be an extension point (people won't subclass it), just a public api (people may need to call methods on it).

himanshug · 2017-07-27T19:37:46Z

@gianm is Task interface not marked as extension point consciously or just missed ? I know at least one user who have written their own task.

We should also mark LookupExtractorFactory and LookupExtractor as extension points. I know of multiple QTL extensions which extend those.

himanshug · 2017-07-27T19:45:59Z

@gianm ServletFilterHolder and PasswordProvider too should be extension points.

gianm · 2017-07-27T23:38:04Z

@gianm is Task interface not marked as extension point consciously or just missed ? I know at least one user who have written their own task.

I consciously didn't add Task because it's not listed on http://druid.io/docs/latest/development/modules.html. And with this PR, I was more trying to encode existing policy in annotations, rather than change the policy. That being said, I would +1 a proposal to make Task an extension point.

We should also mark LookupExtractorFactory and LookupExtractor as extension points. I know of multiple QTL extensions which extend those.

Similar to Task, I left it out since it's not on http://druid.io/docs/latest/development/modules.html. In addition, Lookup APIs are explicitly described as "experimental" in the docs, so I think they shouldn't be annotated as stable. As long as they're experimental, users should be free to make extensions for lookups, but they shouldn't be guaranteed to keep working across upgrades.

@gianm ServletFilterHolder and PasswordProvider too should be extension points.

Hmm, ServletFilterHolder is kind of borderline (it's not on http://druid.io/docs/latest/development/modules.html) but it's close enough to "Jersey resources" (which is) that I guess it makes sense to add. I'll do that.

PasswordProvider isn't on http://druid.io/docs/latest/development/modules.html but maybe it should be. I'll leave the annotation out for now.

gianm · 2017-07-27T23:40:47Z

What do people think about adding these as extension points?

Task
PasswordProvider

They make sense to me but since they aren't currently on the modules.md list I wanted to check for opinions. If we generally think they make sense then I can add them to modules.md and annotate them.

jihoonson · 2017-07-28T01:58:44Z

@gianm @himanshug it makes sense to me.

himanshug · 2017-07-28T14:07:31Z

@gianm PasswordProvider should definitely be an extension point, that was the primary reason for introducing it so that users can integrate Druid with their own internal secrets management systems effectively.
I don't feel so strongly about Task though, haven't seen a general need to extend it beyond one use case.

gianm · 2017-07-28T17:06:12Z

@gianm PasswordProvider should definitely be an extension point, that was the primary reason for introducing it so that users can integrate Druid with their own internal secrets management systems effectively.

I'll make PasswordProvider an extension point too.

I don't feel so strongly about Task though, haven't seen a general need to extend it beyond one use case.

I'll leave Task as "internal" for now, but if there is some consensus that it's a good idea to make it a formal extension point, then I'm ok with changing it.

gianm · 2017-07-28T17:17:16Z

@himanshug @jihoonson @leventov I made the adjustments discussed, and also updated the modules.md doc to reflect what has been annotated (as well as provided some more "hints" for implementors).

Please let me know what you think, especially about the modules.md changes.

himanshug · 2017-07-31T20:06:09Z

👍

drcrallen · 2017-08-21T21:24:24Z

There have been 3 approvals here, it should be good to go after conflicts are resolved.

gianm · 2017-08-25T20:15:36Z

Resolved conflicts.

himanshug · 2017-08-25T20:42:15Z

@gianm feel free to merge whenever you want. it has enough approvals.

gianm · 2017-08-25T20:56:36Z

Ok, I'll merge it after CI is done.

Add @ExtensionPoint and @publicapi annotations.

202c51e

gianm added Design Review Discuss Improvement labels Jun 21, 2017

gianm added 2 commits June 21, 2017 08:17

Clean up wording.

3fbf4bd

Remove unused import.

bea326b

gianm closed this Jun 21, 2017

gianm reopened this Jun 21, 2017

leventov mentioned this pull request Jun 21, 2017

improve query timeout handling and limit max scatter-gather bytes #4229

Merged

Remove unused imports.

100ca4d

leventov reviewed Jul 3, 2017

View reviewed changes

gianm added 2 commits July 5, 2017 08:33

Merge branch 'master' into extension-annotations

06facda

Only types can be extension points.

eed9e1f

leventov approved these changes Jul 7, 2017

View reviewed changes

gianm added this to the 0.11.0 milestone Jul 18, 2017

himanshug reviewed Jul 25, 2017

View reviewed changes

gianm added 3 commits July 25, 2017 11:16

Merge branch 'master' into extension-annotations

ce76f9d

Adjust annotations some more.

9a300b2

Remove unused import.

be8fde6

jihoonson approved these changes Jul 27, 2017

View reviewed changes

himanshug reviewed Jul 27, 2017

View reviewed changes

gianm added 2 commits July 27, 2017 16:35

Merge branch 'master' into extension-annotations

5070451

Make ServletFilterHolder an extension point.

4aa3045

Add a couple extension points, and update docs.

3878d1a

Merge branch 'master' into extension-annotations

73341a7

gianm merged commit 9fbfc1b into apache:master Aug 28, 2017

gianm deleted the extension-annotations branch August 28, 2017 21:51

leventov mentioned this pull request Mar 13, 2019

Mark interfaces which are Druid extension points in code #4044

Closed

Conversation

gianm commented Jun 21, 2017

Uh oh!

leventov commented Jun 21, 2017

Uh oh!

gianm commented Jun 21, 2017

Uh oh!

gianm commented Jun 21, 2017

Uh oh!

fjy commented Jun 23, 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!

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!

gianm commented Jul 5, 2017

Uh oh!

gianm commented Jul 7, 2017

Uh oh!

drcrallen commented Jul 25, 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!

gianm Jul 25, 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!

gianm commented Jul 25, 2017

Uh oh!

gianm commented Jul 27, 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!

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!

himanshug commented Jul 27, 2017

Uh oh!

himanshug commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

gianm Jul 25, 2017 •

edited

Loading

himanshug commented Jul 27, 2017 •

edited

Loading

gianm commented Jul 28, 2017 •

edited

Loading