Sounds good! I will open JIRAs for things I think we can improve. Regarding the code changes... what is going to be the workflow? Have you thought about following the gitflow approach? Something like this: git-flow branching model <http://nvie.com/posts/a-successful-git-branching-model/>
I think your point about a streaming DataSet is very interesting in fact we have already done a pretty simple spike to find out how the memory increases for the MongoDB MM module when querying big tables. Is there any module with a "streaming" DataSet yet? Kind regards, Alberto 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-24 9:38 GMT+02:00 Kasper Sørensen <[email protected]>: > OK, so I'll push that change to the master soon. > > I guess the next step is to start making JIRAs for stuff we want to > improve. That could be to support insert/update/delete (ie. implement > UpdateableDataContext interface) or it could be other improvements (for > instance I noticed that the DataSet doesn't look to be streaming, so large > datasets would cause out of memory exceptions (I think, didn't look very > closely into it yet)). > > You are more than welcome to start putting in those things into JIRA. Then > we have a nice process going where issues can be dealt with individually > and we can track if there's more on our wish-list. > > Best regards, > Kasper > > 2014-09-24 9:27 GMT+02:00 Alberto Rodriguez <[email protected]>: > > > Hi Kasper, > > > > I've been having a look at the changes and they all look fine with me. I > am > > not really used to this kind of revisions...what should be the next > steps? > > > > By the way, I did not have time to work on the insert, delete and update > > operations, do you think we should implement them? > > > > Kind regards, > > > > Alberto > > > > 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-21 22:55 GMT+02:00 Kasper Sørensen < > [email protected] > > >: > > > > > Here's my suggested changes to make it single-index based. I also took > > the > > > liberty to clean up the code style a little to be in line with all the > > > other MM code. Hope that's OK. > > > > > > https://reviews.apache.org/r/25883/ > > > > > > 2014-09-21 22:28 GMT+02:00 Kasper Sørensen < > > [email protected] > > > >: > > > > > > > Yeah, let's make it single-schema oriented as a start. We might later > > add > > > > multi-schema support to it, if we can also get around that in the > > > > superclass. > > > > > > > > The JdbcDataContext does not inherit from QueryPostprocessDataContext > > at > > > > all - it's a completely separate implementation that has all the > schema > > > > model handling and query execution based on JDBC and encoding the > > queries > > > > into SQL. That's of course by far more effecient, but also a lot more > > > work > > > > than just using the QueryPostprocessDataContext's query handling > > > mechanism. > > > > > > > > 2014-09-21 19:55 GMT+02:00 Alberto Rodriguez <[email protected]>: > > > > > > > >> Yes, we might force the user to pass in the index name on the > > > DataContext > > > >> creation. This way, if the user wants to point to another index he > > > should > > > >> create another DataContext which is not so bad IMHO. > > > >> > > > >> I am out of home this weekend and I can't have a look at MM > repo...how > > > do > > > >> you handle this for JDBC? > > > >> > > > >> Kind Regards, > > > >> > > > >> Alberto > > > >> El 21/09/2014 15:18, "Kasper Sørensen" < > > [email protected]> > > > >> escribió: > > > >> > > > >> > Hmm I imagine that we would not be happy with that solution in the > > > long > > > >> run > > > >> > and then would have to change it in the future anyway. And that > kind > > > of > > > >> > change is a bit painful because it's only a runtime backwards > > > >> > incompatibility so people will find out too late. > > > >> > > > > >> > We could also consider to make the implementation index specific, > > ie. > > > >> only > > > >> > for a single index. That would make a lot of things easy to model > > too. > > > >> > Den 21/09/2014 12.29 skrev "Alberto Rodriguez" <[email protected] > >: > > > >> > > > > >> > > Hi Kasper, > > > >> > > > > > >> > > That's great news!! Thank you for merging the code! > > > >> > > > > > >> > > Regarding the issue you found out, as a simple workaround, what > if > > > we > > > >> > > prepend the index name to the "table" name,something like: > > > >> > > indexname.indextable > > > >> > > > > > >> > > Kind regards, > > > >> > > > > > >> > > Alberto > > > >> > > Hi Alberto, > > > >> > > > > > >> > > As you've probably seen, the code has now been merged with the > > main > > > >> > > MetaModel repo - congratulations and thank you a lot! > > > >> > > > > > >> > > After merging it I took it for another spin to really dive deep > > into > > > >> some > > > >> > > of the design choices made. I had an observation (and just > > > committed a > > > >> > > javadoc TODO comment about it) - the approach of mapping indexes > > and > > > >> > > document types is potentially destructable in the sense that if > > you > > > >> have > > > >> > > multiple indexes with the same document types, those table names > > are > > > >> > going > > > >> > > to be in conflict. Everything is right now put under the same > > > schema, > > > >> > while > > > >> > > I think actually the ideal approach would be to produce multiple > > > >> schemas, > > > >> > > with their document types represented as tables. That would > > reflect > > > >> > better > > > >> > > on the hierarchy that ES has. > > > >> > > > > > >> > > Obviously that's harder to do ... And unfortunately right now > the > > > >> > > QueryPostprocessDataContext class dictates that you have to > > produce > > > a > > > >> > > single "main" schema. Which is kinda stupid and circumstantial. > > So I > > > >> > > propose we see if we can fix that, and then make the ES module > > > >> > multi-schema > > > >> > > oriented. > > > >> > > > > > >> > > 2014-09-17 23:51 GMT+02:00 Alberto Rodriguez <[email protected] > >: > > > >> > > > > > >> > > > 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>* > > > >> > > > > > > > >> >> > > > > >> > > > > > > > >> >> > > > >> > > > > > > > >> > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > > > > >
