Hi Kasper, please see my notes on your PR comments, I've also pushed new changes to the repo.
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-17 9:45 GMT+02:00 Kasper Sørensen <[email protected]>: > Hi Alberto, > > I added a bunch of remarks to the PR in github. None of them absolutely > critical to do before we merge it, but nice if you anyway want to spend > more time on it. > > Best regards > Kasper > Den 16/09/2014 23.01 skrev "Henry Saputra" <[email protected]>: > > > 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>* > > >> >> > > > >> >> > > >> > > >
