-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add Rackspace Cloud Files Deep Storage Extension #1719
Conversation
<parent> | ||
<groupId>io.druid</groupId> | ||
<artifactId>druid</artifactId> | ||
<version>0.8.0</version> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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.
Are the tests blocking for accepting the merge? |
@se7entyse7en we need 2 👍s before we can merge |
d27a4d1
to
5e0424b
Compare
|
||
public CloudFilesObject get(String path) { | ||
SwiftObject swiftObject = objectApi.get(path); | ||
Payload payload = swiftObject.getPayload(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
Overall the PR is in a pretty good spot. A few things need addressed still
Eventually the PR will need squashed but not requried until reviews are done |
<version>1.0.13</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>mysql</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required?
e9549f5
to
d3fcd36
Compare
bfe0f54
to
ac823e4
Compare
👍 |
ac823e4
to
4172b58
Compare
I added a comment to the pom file as requested by @xvrl. |
4172b58
to
ea72ec1
Compare
Closed and reopened to restart build. |
Please squash once everything looks good on your end |
ea72ec1
to
24765cf
Compare
@drcrallen ok, squashed into a single commit. |
@se7entyse7en please include se7entyse7en#1 in the squash if possible. |
failure in |
dd5fa05
to
c924f9f
Compare
@drcrallen included and squashed |
…s as deep storage
Cool thanks for your patience in the matter! 👍 after travis |
|
Bouncing for travis |
…les-deep-storage Add Rackspace Cloud Files Deep Storage Extension
Thanks to everyone for the help. |
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):
and then I run the various nodes: