DRILL-5956: Add Storage Plugin for Apache Druid #1888
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the submission!! Please let me know if you need any help with the review process as well. Thanks!! |
cgivre
changed the title
|
@akkapur |
cgivre
force-pushed
the
feature/DRILL-5956
branch
from
7942432
to
d7b02ed
Compare
|
@akkapur I just updated the PR to use the latest build of Drill and rebuilt the protobufs. |
Great!. Thanks for keeping tabs on this. I have been adding more unit tests. |
|
@cgivre I am running into a runtime exception when running the tests in TestDruidQueries.java.
Also, tests in TestDruidQueries.java require a running instance of Druid. |
|
@akkapur |
|
@akkapur However, the unit tests which did require a Druid installation did not seem to work properly. I attempted to run the unit tests with Druid via the docker-composer that you supplied and I could not get it to work. I then attempted to use the instructions here 1 to start a small Druid installation to see if I could connect and also could not. I'm wondering whether the ports in the bootstrap.json file are correct. After enabling the storage plugin, starting Druid and executing a I will continue the first round of code review, but could you please provide some instructions as to how to set up a test environment? (Please keep in mind that I have no experience with Druid) Here is the stack trace for the unit tests: |
|
I suspect the reason the unit tests are failing is that something is not configured correctly in the test environment, but I don't know enough about Druid to debug. |
|
@cgivre thanks for taking time to review. I will try to resolve this asap. |
@cgivre this should be resolved now. Let me know if it works now. I also changed the port mappings for druid to the default ones 808*. |
|
@akkapur I ran the following queries without incident: SHOW DATABASES;
SELECT * FROM druid.wikipedia LIMIT 10;
SELECT user, channel FROM druid.wikipedia LIMIT 10;
SELECT user, channel, bob FROM druid.wikipedia LIMIT 10;
SELECT DISTINCT countryName FROM druid.wikipedia LIMIT 10;
SELECT countryName, COUNT(*) as countryCount
FROM druid.wikipedia
GROUP BY countryName
ORDER BY countryCount DESC;
SELECT countryName, COUNT(*) as countryCount
FROM druid.wikipedia
WHERE countryName IS NOT NULL
GROUP BY countryName
ORDER BY countryCount DESC;I'll start the code review now but this is looking good!!! Thank you for your hard work and patience with this PR. |
For the time being, the tests which depend upon a running instance of druid are marked as ignored and to be run manually. Ideally, i would like to enable them and run them as part of the build. However, it takes too long to download and run druid in a CI build. Would you recommend on how to handle this scenario? |
|
@akkapur Sorry, I misunderstood the intent of that change. The unit tests are very thorough and let's leave them for the time being. |
paul-rogers
requested changes
Mar 14, 2020
...age-druid/src/main/java/org/apache/drill/exec/store/druid/DruidCompareFunctionProcessor.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidSubScan.java
Outdated
Show resolved
Hide resolved
...rib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/common/DruidAndFilter.java
Outdated
Show resolved
Hide resolved
...rib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/common/DruidAndFilter.java
Outdated
Show resolved
Hide resolved
...b/storage-druid/src/main/java/org/apache/drill/exec/store/druid/common/DruidBoundFilter.java
Outdated
Show resolved
Hide resolved
...b/storage-druid/src/main/java/org/apache/drill/exec/store/druid/common/DruidBoundFilter.java
Outdated
Show resolved
Hide resolved
|
@cgivre hi Charles, I am having trouble getting the right version of protobuf to generate the files. Is this something you can help with? Currently, the build seems to be failing because of that. |
|
@akkapur But for now, I'd say don't worry about the protobufs. Let's get the code reviewed and once we're ready to commit we can fix the protobufs. |
cgivre
requested changes
May 11, 2020
There was a problem hiding this comment.
Hi @akkapur
Thank you for all your hard work on this plugin. It is really coming along nicely. I reviewed all the files and posted suggested changes.
I feel like we are getting close, but before we do the next round, can you please:
-
Do a scrub and remove any
TODOcomments in the code? If you want to leave them, please create a JIRA, and reference the JIRA number. Also if there are any commented out blocks, please remove them as well. -
Please verify that all loggers are of the format:
private static final Logger logger = LoggerFactory.getLogger(<myclass>.class);
and that all debug messages are in the format:
logger.debug( "Text blah blah {}", variable). (It's not the message that we care about, but we prefer not to use string concatenation in logging messages) -
For any classes that are serialized using Jackson, please use the
PlanStringBuilderfor those classestoString()method.
In general we're getting close. I ran all the unit tests and all of the tests worked on my machine except those that required Druid to be running. I built Drill and ran the queries in Drill and they worked, so I suspect the issue is with my environment rather than the tests.
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidQueryClient.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidRecordReader.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidRecordReader.java
Outdated
Show resolved
Hide resolved
...rib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidScanBatchCreator.java
Outdated
Show resolved
Hide resolved
...torage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidPushDownFilterForScan.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidFilterBuilder.java
Outdated
Show resolved
Hide resolved
...storage-druid/src/main/java/org/apache/drill/exec/store/druid/schema/DruidSchemaFactory.java
Outdated
Show resolved
Hide resolved
...rib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/rest/DruidQueryClient.java
Outdated
Show resolved
Hide resolved
...ib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/rest/RestClientWrapper.java
Show resolved
Hide resolved
|
@akkapur |
|
Yes, will do this today.
Thanks
…On Tue, May 19, 2020 at 8:26 PM Charles S. Givre ***@***.***> wrote:
@akkapur <https://github.com/akkapur>
I just updated the protobufs and I think they're all up to date. Can you
rebase with the current master?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1888 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAY2XHG4SJN6H7A4A23LAR3RSMPUPANCNFSM4JH66QDA>
.
|
tenghuanhe
reviewed
Jun 7, 2020
...rib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/rest/DruidAdminClient.java
Outdated
Show resolved
Hide resolved
|
@cgivre I added the limit push down you mentioned. |
|
@akkapur |
cgivre
requested changes
Jun 22, 2020
There was a problem hiding this comment.
@akkapur
I had a few very minor suggestions to the RecordReader. Can you please address these, and squash all commits and we're good to go.
Since you've never done this before, the procedure for committing is to squash all the commits and set the commit message to the full PR title: DRILL-5956: Add Storage Plugin for Apache Druid.
Thank you for all your work on this plugin! I have a few suggestions but I think they can wait until this is committed.
...rage-druid/src/test/java/org/apache/drill/exec/store/druid/DruidStoragePluginConfigTest.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java
Show resolved
Hide resolved
...storage-druid/src/test/java/org/apache/drill/exec/store/druid/rest/DruidQueryClientTest.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidRecordReader.java
Outdated
Show resolved
Hide resolved
contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidRecordReader.java
Outdated
Show resolved
Hide resolved
cgivre
approved these changes
Jun 22, 2020
|
@akkapur Thank you for this submission!! |
akkapur
force-pushed
the
feature/DRILL-5956
branch
from
967ed22
to
e69fd4e
Compare
|
Hi @akkapur |
Yes. will do. |
akkapur
force-pushed
the
feature/DRILL-5956
branch
from
e69fd4e
to
a7f2c8f
Compare
|
@akkapur |
Seems like i have the same issue generating the protobuf like the last time. Would be great if you could try on your end. |
|
@akkapur, you may just copy and apply a patch from CI logs: https://github.com/apache/drill/pull/1888/checks?check_run_id=797943288#step:7:10 This job checks whether protobufs are up-to-date and to do so, it regenerates them and checks whether there were no changes. |
|
@akkapur |
|
@vvysotskyi |
|
@cgivre, CI uses protobufs version 3.11.1-1. You may just copy the output of CI starting from |
akkapur
force-pushed
the
feature/DRILL-5956
branch
from
ffd936b
to
7995db2
Compare
@cgivre I just pushed this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Starting work to add a connector for Apache DRUID.
Currently, supports Select queries only.
Files Reviewed:
DruidAndFilter.javaDruidBoundFilter.javaDruidCompareFunctionProcessor.javaDruidFilterBuilder.javaDruidGroupScan.javaDruidScanBatchCreator.javaDruidScanSpecBuilder.java- [ ]DruidScanner.javaDruidStoragePlugin.javaDruidStoragePluginConfig.javaDruidSubScan.javaREADME.md