Hi Kasper, all your suggestions/changes have been done.
I am also happy to be involved in such an interesting project. Kind regards, Alberto 2014-09-17 21:21 GMT+02:00 Kasper Sørensen <[email protected]>: > 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>* > > > > >> >> > > > > > >> >> > > > > >> > > > > > > > > > >
