Hi Charles, That makes sense to me. It would be helpful to use a common HTTP module. I used the `DefaultHttpClient`, which i am not too stoked about.
As long as there is an interface to mock for the new http module, it would work great. - Ankush On Tue, May 5, 2020 at 8:53 AM Charles Givre <cgi...@gmail.com> wrote: > Hi Ankush, > Thanks for all your work on the Druid plugin. I'm going to try to do a > review over the weekend. I had a question/suggestion. Recently we > committed a storage plugin for REST APIs that use the OkHTTP3 library as > the basis for HTTP requests. In the quest to get it committed there was > some discussion about the fact that Drill uses several different libraries > for making HTTP REST requests. > > I wanted to ask your thoughts about perhaps moving this class from the > HTTP plugin to a utility class and then having both the Druid and HTTP > plugin use this class for REST requests? > What do you think? > -- C > > > > On Apr 27, 2020, at 12:55 AM, Ankush Kapur <ankush.ka...@gmail.com> wrote: > > Making good progress with the changes requested from the review(s). > > - Ankush > > On Sun, Apr 19, 2020 at 12:36 AM Charles Givre <cgi...@gmail.com> wrote: > >> I don't mean to be a pain in the you know where about this... I very >> much would like to see this as a part of Drill. >> Best, >> -- C >> >> >> >> On Apr 18, 2020, at 4:54 PM, Ankush Kapur <ankush.ka...@gmail.com> wrote: >> >> Apologies for being MIA, got hung-up at work really bad. >> >> Yes, I did start working on the changes requested in the review. Will act >> on the other things you mentioned, as well. >> >> Thank you for following-up though, helps if someone is watching over :) >> >> - Ankush >> >> On Fri, Apr 17, 2020 at 12:56 AM Charles Givre <cgi...@gmail.com> wrote: >> >>> Hi Ankush, >>> How's it going? I hope you are doing ok with all that is happening >>> around us. I hate to be a pest but I was wondering if you've had the >>> chance to work on the Druid storage plugin review? I'm happy to help with >>> some of the changes, but I didn't want to step on your toes or re-do things >>> you're currently working on. >>> >>> Would it be possible for you to rebase on the current master as well? >>> There have been some improvements unrelated to this plugin which will be >>> helpful as we move towards committing this. >>> Stay safe! >>> >>> -- C >>> >>> >>> >>> >>> >>> On Mar 22, 2020, at 8:34 AM, Ankush Kapur <ankush.ka...@gmail.com> >>> wrote: >>> >>> Yes, thank you. both for your time reviewing... >>> >>> I am working on changes based on it. >>> >>> - Ankush >>> >>> On Fri, Mar 20, 2020 at 11:25 AM Charles Givre <cgi...@gmail.com> wrote: >>> >>>> Hi Ankush. >>>> I hope all is well. Were you able to see Paul and my review comments >>>> for the Druid plugin? >>>> Thanks, >>>> -- C >>>> >>>> On Mar 13, 2020, at 8:17 AM, Charles Givre <cgi...@gmail.com> wrote: >>>> >>>> HI Ankush, >>>> I hope you're doing ok. I live in Maryland and all our schools are now >>>> closed for the next two weeks, so it will be interesting.... Anyway, I >>>> tagged you in a few comments, but it's weird that my comments didn't show >>>> up on the main page. Take a look here and you should see them: >>>> https://github.com/apache/drill/pull/1888/files >>>> >>>> General comments: >>>> 1. Please verify that all logger creations are in the correct format. >>>> Also, I'd strongly suggest avoiding info messages and use either debug, >>>> warn or error as the case may be. >>>> 2. I had a general question about security and passing credentials to >>>> Druid. I don't really know how Druid handles authentication, but it didn't >>>> seem like there was any way to pass creds to Druid. In any event, this >>>> will need to be documented in the README file. >>>> 3. Please go through and remove any commented out code. >>>> >>>> Writing a storage plugin is not easy, and I'm really impressed that >>>> your first contribution to Drill is something of this scale. This is >>>> really nice work and I'm really hoping we can get it committed for the next >>>> release. >>>> Thanks, >>>> -- C >>>> >>>> >>>> On Mar 13, 2020, at 5:56 AM, Ankush Kapur <ankush.ka...@gmail.com> >>>> wrote: >>>> >>>> Hi Charles, >>>> >>>> Things are good here, and hope the same for you and your family. >>>> >>>> Pardon me, but I do not see your review comments on the four files >>>> mentioned. >>>> >>>> - Ankush >>>> >>>> On Thu, Mar 12, 2020 at 8:36 PM Charles Givre <cgi...@gmail.com> wrote: >>>> >>>>> Hi Ankush, >>>>> I hope all is well. I started the review on your plugin. I've >>>>> reviewed about four files so far. When you have a chance, please take a >>>>> look at the review comments and if you have any questions, please let me >>>>> know. >>>>> Best, >>>>> -- C >>>>> >>>>> >>>>> On Feb 28, 2020, at 8:33 AM, Charles Givre <cgi...@gmail.com> wrote: >>>>> >>>>> Hi Ankush, >>>>> I hope all is well. I've been speaking with Paul about your plugin and >>>>> I'm going to do the code review. I usually don't do big code reviews like >>>>> this, so I was hoping someone else would pick it up, but that hasn't >>>>> happened and I really want to see this get committed to the next version >>>>> of >>>>> Drill. >>>>> >>>>> Since this is a large PR, I'm going to do this in small reviews rather >>>>> than one massive review so please expect waves of review. I think it's >>>>> best that way it doesn't get overwhelming. My game plan will be to focus >>>>> on a few files at a time rather than trying to look at the entire plugin >>>>> and give a few comments here and there. I'll send future correspondence >>>>> via the dev alias in the interest of transparency as well. I'm going to >>>>> ask Paul to look at the pushdown code as he is a lot more familiar with >>>>> that than I am, but we're not there yet. >>>>> >>>>> Thanks for all your work and patience on this and I am excited about >>>>> getting it into Drill. I know it will be a benefit for the community. >>>>> Best, >>>>> -- C >>>>> >>>>> >>>>> >>>>> On Jan 21, 2020, at 8:43 AM, Charles Givre <cgi...@gmail.com> wrote: >>>>> >>>>> Hey Ankush, >>>>> See responses inline. >>>>> >>>>> >>>>> On Jan 21, 2020, at 8:12 AM, Ankush Kapur <ankush.ka...@gmail.com> >>>>> wrote: >>>>> >>>>> Hi Charles, >>>>> >>>>> Thanks for taking time to make the changes. >>>>> >>>>> >>>>> No problem. Also FYI, Drill uses 2 space indents not 4. I just >>>>> pushed an update with all the spacing on 2 space. Is there anything that >>>>> I >>>>> can assist with? >>>>> >>>>> >>>>> Could you please point me to the "commented-out" code you are >>>>> referring too. That's actually an issue i was trying to track down last >>>>> weekend while writing tests. I noticed that the "where" clause was not >>>>> being pushed down. This was keeping me from writing the tests I have been >>>>> working on. >>>>> >>>>> >>>>> The commented out code was in the DruidStoragePlugin, but it does not >>>>> appear to be commented out anymore. Have you rebased on the most current >>>>> branch? When I attempted to build your branch, I got an error in the >>>>> RecordReader class. I'll take another look today or tomorrow. Regarding >>>>> pushdowns, have you seen the Calcite adapter for Druid ( >>>>> https://calcite.apache.org/docs/druid_adapter.html). I'm wondering >>>>> if you can basically extend or import the pushdowns from the Calcite >>>>> adapter so that you don't have to recreate all that. >>>>> >>>>> I've been working on a pluign for Cassandra and am attempting to do >>>>> that as well. Take a look at the Drill JDBC Storage Plugin because I think >>>>> that's sort of what it does. Potentially this could save you a lot of >>>>> work. >>>>> >>>>> >>>>> Also, can you provide a link to the file where it's failing to compile >>>>> for you? I was not able to repro this. >>>>> >>>>> >>>>> The specific error I am getting is on line 151 in the >>>>> DruidRecordReader. The error is that writer is a VectorContainerWriter >>>>> and >>>>> the required type is a ComplexWriter. >>>>> >>>>> >>>>> Fyi, I pushed in some changes I was working on recently, still more >>>>> work to do on the testing side, though. >>>>> >>>>> >>>>> Awesome! >>>>> >>>>> >>>>> Sincerely, >>>>> Ankush >>>>> >>>>> On Mon, Jan 20, 2020 at 10:01 PM Charles Givre <cgi...@gmail.com> >>>>> wrote: >>>>> >>>>>> Hi Ankush, >>>>>> I have a question for you. I was looking at your plugin and it looks >>>>>> like you commented out the bit which causes the filter pushdown to >>>>>> happen. >>>>>> Is this not working properly? >>>>>> >>>>>> Also, I attempted to build it and I got a compilation error in the >>>>>> RecordReader, line 151. I corrected the license issues in the test >>>>>> suite, >>>>>> and pushed that to your branch so hopefully it will pass TravisCI. >>>>>> >>>>>> Best, >>>>>> -- C >>>>>> >>>>>> >>>>>> >>>>>> On Jan 12, 2020, at 10:37 AM, Charles Givre <cgi...@gmail.com> wrote: >>>>>> >>>>>> Great to hear! I'm excited about getting this committed into Drill! >>>>>> -- C >>>>>> >>>>>> On Jan 12, 2020, at 9:06 AM, Ankush Kapur <ankush.ka...@gmail.com> >>>>>> wrote: >>>>>> >>>>>> Hi Charles, >>>>>> >>>>>> Thanks for reaching out. Hope you are having a good 2020 as well. >>>>>> >>>>>> Yes, I am actively working on putting together more tests for the >>>>>> druid storage plugin. >>>>>> >>>>>> I will have something ready very soon, and also make sure the style >>>>>> guide is followed. >>>>>> >>>>>> Will get back soon. >>>>>> >>>>>> Have a wonderful day. >>>>>> >>>>>> Sincerely, >>>>>> Ankush >>>>>> >>>>>> On Fri, Jan 10, 2020 at 10:08 AM Charles Givre <cgi...@gmail.com> >>>>>> wrote: >>>>>> >>>>>>> Hi Ankush, >>>>>>> How's it going? I hope you had a good new year! I wanted to follow >>>>>>> up with you to see if you've had a chance to work on the Druid storage >>>>>>> plugin. I don't mean to be a pest, but I'm excited about this >>>>>>> capability >>>>>>> and would love to see it integrated into Drill. >>>>>>> >>>>>>> Regarding unit tests, I'm currently working on an Elasticsearch >>>>>>> storage plugin for Drill ( >>>>>>> https://github.com/cgivre/drill/tree/storage-elastic2/contrib/storage-elastic/src/test/java/org/apache/drill/exec/store/elasticsearch) >>>>>>> that someone else wrote a few years ago. I got it to work with ES 5.6 >>>>>>> and >>>>>>> Drill 1.17. If you want to borrow the unit tests, please feel free. >>>>>>> >>>>>>> One other thing that I know the reviewers will say is that Drill has >>>>>>> a formatter ( >>>>>>> https://drill.apache.org/docs/apache-drill-contribution-guidelines/) >>>>>>> for source code (spacing, etc.). Could you please run your code through >>>>>>> that to make sure that the style is consistent with Drill? >>>>>>> >>>>>>> If you need any other assistance, please let me know. Thanks! >>>>>>> -- C >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Dec 15, 2019, at 10:13 PM, Ankush Kapur <ankush.ka...@gmail.com> >>>>>>> wrote: >>>>>>> >>>>>>> Hi Charles, >>>>>>> >>>>>>> 1. Yes, I was referring the pushing down aggregations to native >>>>>>> druid aggregation queries. Also, DRUID now supports a new flavour of >>>>>>> "SELECT" query called, "SCAN" query, which is expected to be more memory >>>>>>> efficient. But I will leave for another revision. >>>>>>> >>>>>>> 2. I will add more unit tests. Also will add integration tests to >>>>>>> exercise the queries. >>>>>>> >>>>>>> - Ankush >>>>>>> >>>>>>> On Mon, Dec 9, 2019 at 7:23 PM Charles Givre <cgi...@gmail.com> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Ankush, >>>>>>>> Thanks for the contribution!! This looks like a great start. I >>>>>>>> have a few questions: >>>>>>>> 1. When you say it only supports SELECT queries, what exactly do >>>>>>>> you mean here? Just FYI, most storage plugins are query-only, so most >>>>>>>> only >>>>>>>> support SELECT queries. If you are referring to pushing down GROUP BY >>>>>>>> and >>>>>>>> the like to the plugin, then I think that is fine to worry about later >>>>>>>> ;-). >>>>>>>> If your plugin works with SELECT queries, Drill should be able to >>>>>>>> perform >>>>>>>> the aggregation after the query has executed. >>>>>>>> >>>>>>>> 2. I saw there is only one file of unit tests. I know that the >>>>>>>> first review comment will be that you need more unit tests. Please >>>>>>>> take a >>>>>>>> look at the other plugins and see what kind of unit tests they have. >>>>>>>> At a >>>>>>>> minimum, I would expect to see some query tests. I'm wondering though, >>>>>>>> what is the best way to test this? >>>>>>>> >>>>>>>> Best, >>>>>>>> -- C >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Dec 8, 2019, at 5:36 PM, Ankush Kapur <ankush.ka...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>> Hi Charles, >>>>>>>> >>>>>>>> Apologies for the late reply. Been some crazy times here. >>>>>>>> >>>>>>>> I had the plugin working again and it supports the select queries( >>>>>>>> https://druid.apache.org/docs/latest/querying/select-query.html). >>>>>>>> https://github.com/apache/drill/pull/1888 >>>>>>>> >>>>>>>> Let me know if this is good for an MVP. >>>>>>>> >>>>>>>> People at DRUID now support a lot more rich queries(Scan, >>>>>>>> Aggregations, SQL syntax) since I first wrote the plugin. So I am >>>>>>>> thinking >>>>>>>> future changes would be required, which I would love to work on, >>>>>>>> iteratively. >>>>>>>> >>>>>>>> Let me know if this works for you. >>>>>>>> >>>>>>>> Sincerely, >>>>>>>> Ankush >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Dec 3, 2019 at 11:38 AM Charles Givre <cgi...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi Ankush, >>>>>>>>> How's it going? I thought I'd check in with you to see how the >>>>>>>>> plugin is coming and if you've made any progress? There is currently a >>>>>>>>> change freeze because we're getting ready to release Drill 1.17, but >>>>>>>>> I'd >>>>>>>>> really love to see this get committed for version 1.18. If you're >>>>>>>>> stuck or >>>>>>>>> need assistance, please let me know. >>>>>>>>> Thanks! >>>>>>>>> -- C >>>>>>>>> >>>>>>>>> >>>>>>>>> On Nov 1, 2019, at 3:26 PM, Ankush Kapur <ankush.ka...@gmail.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Hi Charles, >>>>>>>>> >>>>>>>>> I just started a draft PR for this work. >>>>>>>>> https://github.com/apache/drill/pull/1888 >>>>>>>>> >>>>>>>>> >>>>>>>>> Sincerely, >>>>>>>>> Ankush >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Sat, Oct 12, 2019 at 3:30 PM Ankush Kapur < >>>>>>>>> ankush.ka...@gmail.com> wrote: >>>>>>>>> >>>>>>>>>> Hi Charles, >>>>>>>>>> >>>>>>>>>> I apologize for such a late reply. I have been caught up with >>>>>>>>>> product releases at work. >>>>>>>>>> >>>>>>>>>> I will begin working on the PR. Hopefully, it should not be too >>>>>>>>>> long. >>>>>>>>>> >>>>>>>>>> Hope you have a wonderful weekend. >>>>>>>>>> >>>>>>>>>> Sincerely, >>>>>>>>>> Ankush >>>>>>>>>> >>>>>>>>>> On Thu, Sep 19, 2019 at 10:16 AM Charles Givre <cgi...@gmail.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Hi Ankush, >>>>>>>>>>> Thanks for getting back with me! I'm really glad you're open to >>>>>>>>>>> contributing this to Drill! I'm also really impressed that you >>>>>>>>>>> were able >>>>>>>>>>> to do this at all. Writing storage plugins is extremely difficult, >>>>>>>>>>> especially given the lack of documentation, and that you have to >>>>>>>>>>> have a >>>>>>>>>>> really good understanding of both Druid and Drill's internals. So >>>>>>>>>>> my >>>>>>>>>>> hat's off to you! >>>>>>>>>>> Anyway, I had a look at our JIRA board and it turns out that >>>>>>>>>>> there is an open JIRA for a Drill/Druid integration: >>>>>>>>>>> https://issues.apache.org/jira/browse/DRILL-5956 >>>>>>>>>>> >>>>>>>>>>> When you're ready to do so, please create a pull request on the >>>>>>>>>>> Drill github repo with the title: Drill-5956: Add Storage Plugin >>>>>>>>>>> for Druid >>>>>>>>>>> and add me as a reviewer. Thank you very much!! >>>>>>>>>>> Best, >>>>>>>>>>> -- C >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Sep 17, 2019, at 5:23 PM, Ankush Kapur < >>>>>>>>>>> ankush.ka...@gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Charles, >>>>>>>>>>> >>>>>>>>>>> I am good, thanks for asking. Hope the same for you. >>>>>>>>>>> >>>>>>>>>>> Yes, I would be happy to submit a pr. However. It will take >>>>>>>>>>> sometime to get it in shape. The druid connector is a couple of >>>>>>>>>>> years old >>>>>>>>>>> now. >>>>>>>>>>> >>>>>>>>>>> - Ankush >>>>>>>>>>> >>>>>>>>>>> On Tue, Sep 17, 2019, 2:27 PM Charles Givre <cgi...@gmail.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi Ankush, >>>>>>>>>>>> I hope all is well. I'm the PMC chair for Apache Drill and I I >>>>>>>>>>>> saw your Druid plugin on github. I wanted to ask if you would >>>>>>>>>>>> consider >>>>>>>>>>>> contributing that to Drill and submitting a pull request. I'm >>>>>>>>>>>> happy to >>>>>>>>>>>> help with the review process. >>>>>>>>>>>> Thanks, >>>>>>>>>>>> -- Charles >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >> >
