Move GCP to a core extension by drcrallen · Pull Request #6953 · apache/druid

drcrallen · 2019-01-30T00:25:52Z

Special changes:

Move google extension to a core extension
Create aws-common and gcp-common. Put them in server
Move ec2 autoscaler to its own extension
Fix up default extension inclusions
Fix up documentation

As a side note, it seems like the EC2 autoscaling stuff is not actually documented anywhere.

gianm

@drcrallen, apologies for not having a chance to get to this til now. Would definitely love to see it happen.

gianm · 2019-03-05T19:08:04Z

+  {
+    GoogleCredential credential = GoogleCredential.getApplicationDefault(transport, factory);
+    if (credential.createScopedRequired()) {
+      credential = credential.createScoped(Collections.singleton("https://www.googleapis.com/auth/cloud-platform"));


Is there any value in allowing the URL to be configurable?

Since people can now override HttpRequestInitializer in an extension, they can do so with a custom extension. This is more laborious than a simple config change though. For a simple config change it could make sense but I don't have a specific use case in mind and would rather leave such a feature add to someone who understands the value of such a feature better.

gianm · 2019-03-05T19:08:27Z

+  @Override
+  public void configure(Binder binder)
+  {
+


nit: would be nice to have a comment that this is intentionally blank, like // Nothing to do. (Whenever reading empty methods I always wonder if it was intentional or not.)

gianm · 2019-03-05T19:14:22Z

        </dependency>
+        <dependency>
+            <groupId>org.apache.druid</groupId>
+            <artifactId>druid-gcp-common</artifactId>


I wish we didn't have to include gcp-common in the standard lib directory, even for people that don't use GCP. It feels like continuing the sins of our forebears (aws-common, that is). Which / how many jars does this end up adding? Trying to convince myself to be okay with this and not fight it :)

I think you said you tried doing a transitive dependency but had issues with it. For sake of getting it written down - what'd you try & what went wrong?

I'll get a list of dependencies via the maven dependency tree here.

As far as transitive dependency, this goes back to "an extension can't depend on another extension" problem, where the implementation in an extension classloader is not accessible from any other extension. So if I put the AWS injectables in an extension, there is no other extension that can use those. All the overrides must be in the same extension.

As written elsewhere, the only place this does not apply is for things used in the hadoop world, where everything is mangled onto the same ClassLoader (often leading to all kinds of version problems). Since things in the hadoop world get mangled all together, they all can reach each other in the same classloader.

The only "real" solution I can think of for this is to have an Extension Class Loader, whereby an extension can declare a class as "exported" and have that classloader make sure any other extensions can use that common exported class from a single classloader.

Would it work to add aws-common and gcp-common as dependencies for each extension individually, not as their own extensions? So we aren't doing extension-depends-on-extension, but are instead loading the aws/gcp common jars, separately, into each extension classloader?

If they only provided libraries and no bindings it would. But with that approach it does not allow me to make a third party extension to inject new overrides to the HttpInitializer (for example).

And since things are bound guice makes the classloader isolation items weird for classes named the same thing but in different classloaders.

if I recall it fails with a "you bound HttpIntializer multiple times" kind of error

from the extension view point, each implementation of DruidModule is unique in its classloader, so its bindings fire independently.

I see. That makes sense and is a bummer. But thanks for explaining it. I also guess that doing it the way I suggested would mean every extension gets its own GCP client, which is not ideal. OK, I give up, let's do it this way and hopefully someone has a flash of inspiration around making extensions able to depend on other extensions.

I built a tarball to see what new stuff this would add to lib/, and got:

./lib/checker-qual-2.5.7.jar ./lib/druid-gcp-common-0.15.0-incubating-SNAPSHOT.jar ./lib/google-api-client-1.22.0.jar ./lib/google-http-client-1.22.0.jar ./lib/google-http-client-jackson2-1.22.0.jar ./lib/google-oauth-client-1.22.0.jar ./lib/jackson-module-guice-2.6.7.jar

Not too bad! I was afraid it would pull in a bunch of extra deps, but, it doesn't.

I'm super careful dude :)

I'm just paranoid - thanks for indulging :)

gianm · 2019-03-05T19:15:15Z

        <module>extensions-core/protobuf-extensions</module>
        <module>extensions-core/lookups-cached-global</module>
        <module>extensions-core/lookups-cached-single</module>
+        <module>extensions-core/ec2-extensions</module>


This is definitely a nice change. The EC2 autoscaling stuff is better suited to an extension than to core. Hopefully it opens the door to removing Druid's dependency on aws-common.

gianm · 2019-03-07T01:50:44Z

@drcrallen in extensions-core/google-extensions/pom.xml, the groupId's gotta be org.apache.druid.extensions

gianm · 2019-03-07T19:30:14Z

@drcrallen Just saw you push a commit; the groupId in extensions-core/google-extensions/pom.xml still needs adjustment to org.apache.druid.extensions.

drcrallen · 2019-03-12T02:05:29Z

@gianm green now

clintropolis

LGTM 👍

Also hopeful that someone thinks of a clever way to have extensions depend on each other 🤞

drcrallen · 2019-03-12T16:41:28Z

I still need to update the docs before merging

drcrallen · 2019-03-18T20:53:39Z

Related: #7218

drcrallen · 2019-03-25T20:45:55Z

Ping to keep open. need to resolve conflicts and change docs. Trying to get that done soon

clintropolis

LGTM 👍. I think we should probably make follow-up issues about creating docs for druid-ec2-extensions and finding a way to share common extensions with other extensions after this is merged.

clintropolis · 2019-03-27T06:38:18Z

 |druid-parquet-extensions|Support for data in Apache Parquet data format. Requires druid-avro-extensions to be loaded.|[link](../development/extensions-core/parquet.html)|
 |druid-protobuf-extensions| Support for data in Protobuf data format.|[link](../development/extensions-core/protobuf.html)|
 |druid-s3-extensions|Interfacing with data in AWS S3, and using S3 as deep storage.|[link](../development/extensions-core/s3.html)|
+|druid-ec2-extensions|Interfacing with AWS EC2 for autoscaling middle managers|UNDOCUMENTED|


This is I guess sort of documented at ../../configuration/index.html#autoscaler, but I think it would probably be better in the long run to make a doc for this extension and link those autoscaler docs to it. We should also mention in the existing autoscaler docs in the giant configuration page that the druid-ec2-extensions need to be loaded for it to work, but I don't really consider either of these blockers for this PR to get merged since it isn't documented super well in the first place.

* Move GCP to a core extension * Don't provide druid-core >.< * Keep AWS and GCP modules separate * Move AWSModule to its own module * Add aws ec2 extension and more modules in more places * Fix bad imports * Fix test jackson module * Include AWS and GCP core in server * Add simple empty method comment * Update version to 15 * One more 0.13.0-->0.15.0 change * Fix multi-binding problem * Grep for s3-extensions and update docs * Update extensions.md

jihoonson · 2019-06-06T19:16:26Z

Hi @drcrallen, I'm writing a draft of 0.15.0 release notes. Is there anything should be mentioned in the release notes other than that druid-google-extensions extension is now a core extension and bundled in the binary distribution by default?

Charles Allen added 2 commits January 29, 2019 16:17

Move GCP to a core extension

21c31a7

Don't provide druid-core >.<

b9d2b96

drcrallen added the Discuss label Jan 30, 2019

Charles Allen added 6 commits January 29, 2019 16:35

Keep AWS and GCP modules separate

21e1f2d

Move AWSModule to its own module

a8f322a

Add aws ec2 extension and more modules in more places

0bd304b

Fix bad imports

d463c00

Fix test jackson module

05b7273

Include AWS and GCP core in server

fbbb666

drcrallen requested a review from gianm January 30, 2019 20:47

gianm added the Release Notes label Mar 5, 2019

gianm reviewed Mar 5, 2019

View reviewed changes

Charles Allen added 4 commits March 6, 2019 12:46

Merge remote-tracking branch 'apache/master' into gcp/core

cfb59be

Add simple empty method comment

371c66c

Update version to 15

23f2842

One more 0.13.0-->0.15.0 change

972d5f6

Fix multi-binding problem

788e972

clintropolis approved these changes Mar 12, 2019

View reviewed changes

Charles Allen added 3 commits March 25, 2019 13:56

Merge remote-tracking branch 'apache/master' into gcp/core

47e6a8d

Merge remote-tracking branch 'apache/master' into gcp/core

56c6c43

Grep for s3-extensions and update docs

50fba40

drcrallen removed the Discuss label Mar 26, 2019

Update extensions.md

15b58ce

clintropolis approved these changes Mar 27, 2019

View reviewed changes

gianm added this to the 0.15.0 milestone Mar 27, 2019

gianm merged commit eeb3dbe into apache:master Mar 27, 2019

drcrallen deleted the gcp/core branch March 27, 2019 16:07

drcrallen mentioned this pull request Mar 27, 2019

[PROPOSAL] More documentation on ec2 autoscaler #7355

Open

jihoonson mentioned this pull request Mar 27, 2019

Broken master build #7357

Closed

clintropolis mentioned this pull request Mar 27, 2019

fix build #7358

Merged

glasser mentioned this pull request May 11, 2019

google-extensions: upgrade google-http-client, fix "logs (last 8kb)" #7218

Closed

jihoonson mentioned this pull request Jun 8, 2019

0.15.0-incubating release notes #7854

Closed

clintropolis mentioned this pull request Sep 12, 2019

move google ext docs from contrib to core #8512

Merged

gianm mentioned this pull request Dec 4, 2019

Bring GCP functionality to core functionality #6910

Closed

Conversation

drcrallen commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm left a comment

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!

gianm commented Mar 7, 2019

Uh oh!

gianm commented Mar 7, 2019

Uh oh!

drcrallen commented Mar 12, 2019

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Mar 12, 2019

Uh oh!

drcrallen commented Mar 18, 2019

Uh oh!

drcrallen commented Mar 25, 2019

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drcrallen commented Jan 30, 2019 •

edited

Loading