Looks like good changes :-) Happy to co-develop some of this. I made a few further remarks, but so far looking great!
2014-09-17 10:56 GMT+02:00 Alberto Rodriguez <[email protected]>: > 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>* > > > >> >> > > > > >> >> > > > >> > > > > > >
