Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Rackspace Cloud Files Deep Storage Extension #1719

Merged

Conversation

se7entyse7en
Copy link
Contributor

This is the first version to support Rackspace's Cloud Files as druid deep storage. It still lacks of tests. As already discussed here https://groups.google.com/forum/#!topic/druid-development/9O5NMo2-4WU I would also like some suggestions about the pom.xml as I'm having some difficulties in order to build druid along with the cloud-files extension. Here's my current working solution (from the root of the project):

mvn install -DskipTests=true -Dmaven.javadoc.skip=true
cp -f services/target/druid-services-0.8.2-SNAPSHOT-selfcontained.jar /usr/local/druid/lib
java "-Ddruid.extensions.coordinates=[\"io.druid.extensions:druid-kafka-eight:0.8.2-SNAPSHOT\",\"io.druid.extensions:mysql-metadata-storage:0.8.2-SNAPSHOT\",\"io.druid.extensions:druid-cloudfiles-extensions:0.8.2-SNAPSHOT\"]" -Ddruid.extensions.localRepository=/usr/local/druid/repository "-Ddruid.extensions.remoteRepositories=[\"file:///root/.m2/repository/\",\"http://repo1.maven.org/maven2/\",\"https://metamx.artifactoryonline.com/metamx/pub-libs-releases-local\"]" -cp /usr/local/druid/lib/* io.druid.cli.Main tools pull-deps
cd extensions/cloudfiles-extensions/
mvn dependency:copy-dependencies "-DoutputDirectory=/usr/local/druid/jclouds"
cd ../..

and then I run the various nodes:

java -Xmx512m -Duser.timezone=UTC -Dfile.encoding=UTF-8 -cp config/_common:config/coordinator:/usr/local/druid/lib/*:/usr/local/druid/jclouds/* io.druid.cli.Main server coordinator
java -Xmx512m -Duser.timezone=UTC -Dfile.encoding=UTF-8 -cp config/_common:config/historical:/usr/local/druid/lib/*:/usr/local/druid/jclouds/* io.druid.cli.Main server historical
java -Xmx512m -Duser.timezone=UTC -Dfile.encoding=UTF-8 -cp config/_common:config/broker:/usr/local/druid/lib/*:/usr/local/druid/jclouds/* io.druid.cli.Main server broker
java -Xmx512m -Duser.timezone=UTC -Dfile.encoding=UTF-8 -Ddruid.realtime.specFile=examples/kafka/kafka_realtime.spec -cp config/_common:config/realtime:/usr/local/druid/lib/*:/usr/local/druid/jclouds/* io.druid.cli.Main server realtime

<parent>
<groupId>io.druid</groupId>
<artifactId>druid</artifactId>
<version>0.8.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be updated as the PR moves along. Current master is 0.8.2-SNAPSHOT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed it while cleaning the branch as I started this feature from 0.8.0, I'm fixing.

@drcrallen drcrallen changed the title [WIP] Feature Rackspace Cloud Files Deep Storage Add Rackspace Cloud Files Deep Storage Extension Sep 10, 2015
@drcrallen
Copy link
Contributor

@se7entyse7en change the parent of your extension to match the master pom. Then you should be able to use it like any other extension coordinate


/**
*/
public class CloudFilesDataSegmentPusherConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotNull annotation but no enforcement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Can you elaborate please? Do you perhaps mean that they're NotNull, but they don't have a default argument such as empty string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you still need Preconditions.checkNotNull in the getter (or in a constructor for eager failures) Jackson doesn't really enforce NotNull or @JsonProperty(required = true).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added Preconditions.checkNotNull in the getters.

@se7entyse7en
Copy link
Contributor Author

Are the tests blocking for accepting the merge?

@fjy
Copy link
Contributor

fjy commented Sep 11, 2015

@se7entyse7en we need 2 👍s before we can merge

@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch 2 times, most recently from d27a4d1 to 5e0424b Compare September 11, 2015 13:26

public CloudFilesObject get(String path) {
SwiftObject swiftObject = objectApi.get(path);
Payload payload = swiftObject.getPayload();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

payload is closeable but never closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added a closeStream method to CloudFilesByteSource and I added the closing in getSegmentFiles in CloudFilesDataSegmentPuller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make sure there is a unit test that ensures payload is closed?

@drcrallen
Copy link
Contributor

Overall the PR is in a pretty good spot. A few things need addressed still

  1. Lots of newlines at end of file are missing
  2. pom.xml for extension needs trimmed to only include stuff that is required rather than taking the example pom.xml directly
  3. How users are supposed to handle the different jclouds providers : https://jclouds.apache.org/reference/providers/ Having docs on which ones are supported and which ones are not and how to enable/use them should be mentioned either in docs or at a minimal in the pom.xml.
  4. Docs on configuration options in docs/content/dependencies/deep-storage.md

Eventually the PR will need squashed but not requried until reviews are done

<version>1.0.13</version>
</dependency>
<dependency>
<groupId>mysql</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required?

@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch from e9549f5 to d3fcd36 Compare September 14, 2015 08:58
@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch 2 times, most recently from bfe0f54 to ac823e4 Compare November 3, 2015 15:50
@fjy
Copy link
Contributor

fjy commented Nov 4, 2015

👍

@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch from ac823e4 to 4172b58 Compare November 4, 2015 10:29
@se7entyse7en
Copy link
Contributor Author

I added a comment to the pom file as requested by @xvrl.

@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch from 4172b58 to ea72ec1 Compare November 4, 2015 11:17
@se7entyse7en se7entyse7en reopened this Nov 4, 2015
@se7entyse7en
Copy link
Contributor Author

Closed and reopened to restart build.

@drcrallen
Copy link
Contributor

@drcrallen
Copy link
Contributor

Please squash once everything looks good on your end

@drcrallen drcrallen removed the Discuss label Nov 4, 2015
@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch from ea72ec1 to 24765cf Compare November 4, 2015 16:12
@se7entyse7en
Copy link
Contributor Author

@drcrallen ok, squashed into a single commit.

@drcrallen
Copy link
Contributor

@se7entyse7en please include se7entyse7en#1 in the squash if possible.

@drcrallen
Copy link
Contributor

failure in JDBCExtractionNamespaceTest.testFindNew:261->assertUpdated:312 update check expected:<ba[z]> but was:<ba[r]> not a concern

@se7entyse7en se7entyse7en force-pushed the feature-rackspace-cloud-files-deep-storage branch from dd5fa05 to c924f9f Compare November 4, 2015 16:39
@se7entyse7en
Copy link
Contributor Author

@drcrallen included and squashed

@drcrallen
Copy link
Contributor

Cool thanks for your patience in the matter!

👍 after travis

@drcrallen
Copy link
Contributor

AnnouncerTest.testSessionKilled:168 » NoNode KeeperErrorCode = NoNode for /tes...

@drcrallen
Copy link
Contributor

Bouncing for travis

@drcrallen drcrallen closed this Nov 4, 2015
@drcrallen drcrallen reopened this Nov 4, 2015
@drcrallen drcrallen closed this Nov 4, 2015
@drcrallen drcrallen reopened this Nov 4, 2015
fjy added a commit that referenced this pull request Nov 4, 2015
…les-deep-storage

Add Rackspace Cloud Files Deep Storage Extension
@fjy fjy merged commit 1cbc514 into apache:master Nov 4, 2015
@se7entyse7en
Copy link
Contributor Author

Thanks to everyone for the help.

@fjy fjy mentioned this pull request Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants