Thanks Alberto, this should help review the patch - Henry
On Tue, Sep 16, 2014 at 1:44 PM, Alberto Rodriguez <[email protected]> wrote: > Hi Henry, > > I have just submitted a PR against the MM github mirror. > > Kind regards, > > Alberto Rodríguez > > 2014-09-16 19:01 GMT+02:00 Henry Saputra <[email protected]>: > >> Alberto, >> >> Thanks for the ES module patch. >> >> Could you submit PR against Apache MetaModel github mirror? >> We may need to manually merge your fixes to ASF Git repo later but >> Github PR will help with reviews. >> >> - Henry >> >> On Tue, Sep 16, 2014 at 2:45 AM, Alberto Rodriguez >> <[email protected]> wrote: >> > Hi Kasper, >> > >> > thank you very much for your feedback! >> > >> > I have just modified the getMainSchemaName method to get the cluster name >> > from ES. Regarding the executeQuery method I commented it out because >> some >> > tests were failing, anyway I will try to make this method work and get >> all >> > the benefits from the awesome elasticsearch java API. >> > >> > My apache username is: albertostratio >> > >> > Kind regards, >> > >> > >> > Alberto Rodríguez >> > >> > >> > <http://www.stratio.com/> >> > Avenida de Europa, 26. Ática 5. 3ª Planta >> > 28224 Pozuelo de Alarcón, Madrid >> > Tel: +34 91 352 59 42 // *@stratiobd <https://twitter.com/StratioBD>* >> > >> > 2014-09-16 10:45 GMT+02:00 Kasper Sørensen < >> [email protected]>: >> > >> >> Hi Alberto, >> >> >> >> Once again, thank you very much for this great work. I think this >> module is >> >> looking 99% ready to be introduced into the main MetaModel codebase. >> >> >> >> Here are my very minor remarks: >> >> >> >> >> >> - The main schema name "ElasticSearchSchema" is hardcoded and not >> >> necesarily what users would want. I was thinking if we should either: >> >> - Make it parameterizable in the constructor (default value would IMO >> >> better be just "ElasticSearch" which is in line with "MongoDB", >> >> "CouchDB" >> >> etc.) >> >> - Use the ES cluster name. This would give a more "native" >> feeling - >> >> then we've mapped it to something that is recognizable from the ES >> >> connection. >> >> - Or a combination of the above - make a constructor name >> available, >> >> but if null we use the cluster name. >> >> - You've now commented out your whole executeQuery(Query) method. >> Might >> >> be on purpose, but I guess you had some nice optimizations for >> special >> >> cases in there. I hope we can restore that. >> >> >> >> What is your apache username? When we have that, I propose that we take >> >> your contribution for a VOTE and then hopefully integrate it into the >> main >> >> codebase of MM. The corrections above can be made either before or after >> >> IMO. >> >> >> >> Best regards, >> >> Kasper >> >> >> >> >> >> 2014-09-16 9:28 GMT+02:00 Alberto Rodriguez <[email protected]>: >> >> >> >> > Hi all, >> >> > >> >> > I have been working on the ES module. As Kasper suggested I have >> >> overriden >> >> > the materializeMainSchemaTable to get a first functional version. I >> have >> >> > added quite a few tests to check that the module is working fine for >> >> > *SELECT* operations. I have also implemented the executeCountQuery >> >> method. >> >> > >> >> > I forked the project from the *master branch, *you can check out the >> >> > changes here: incubator-metamodel fork with ES module >> >> > <https://github.com/albertostratio/incubator-metamodel> >> >> > >> >> > I have also been informed by the ASF that my ICLA has been filed in >> their >> >> > records. >> >> > >> >> > Kind regards, >> >> > >> >> > >> >> > Alberto Rodríguez >> >> > >> >> > >> >> > <http://www.stratio.com/> >> >> > Avenida de Europa, 26. Ática 5. 3ª Planta >> >> > 28224 Pozuelo de Alarcón, Madrid >> >> > Tel: +34 91 352 59 42 // *@stratiobd <https://twitter.com/StratioBD>* >> >> > >> >> >>
