Daniel the only thing we're changing is that I am reverting the MR, and creating a new MR for the engine-server renaming changes. So that we can give more tests to it.
We do have a docker image that supports running Marvin and R right now. There is no way I can even test an R engine while we don't have an R common lib, which is part of https://issues.apache.org/jira/browse/MARVIN-3 . This refactoring is being done in parts, not all at once. All the other items I believe are waste of time discussing, given my MR was standing for 2 weeks and got approved recently. If you figured out a better way of doing MARVIN-1, please send us an MR and we can discuss that. Regards, Lucas On Thu, May 23, 2019 at 1:19 PM Daniel Takabayashi < daniel.takabaya...@gmail.com> wrote: > So, guys, we have three actions over here, and I think we should vote ASAP > to avoid uselessly efforts. > > 1 - Revert the PR#21 > 2 - Change the name from engine-executor to engine-server > 3 - New PR to allow us to test an review of the new R support. > > +1 for number 1 > Justification: Because I believe is the best way to avoid future problems > and to give us a chance to do a better code review > > -1 for number 2 > Justification: I don't see any reason to do this, also the efforts to > change, test and validate everything are huge. Besides the fact that we > going to lose a lot of external references from articles, presentations, > manuals and etc. > > +1 for number 3 but > Justification: The only way this makes sense for me it is following the > decisions we have made, using as reference the architecture designed before > (link in my previous email). Following this rationale, what Lucas was > trying to merge should be this component: > > *item 5 - Create an empty engine template (r-engine-template) to be used to > create new engines.* > > > Sorry guys to be so restricted in this case asking to follow the previous > design but once there is a bunch of new pieces around this new feature. Not > respecting these rules will make very hard for us to archive our goals once > the communication will change after each newly merged code. And Lucas, I am > sorry it is not about your code I think the misunderstanding here was about > the task at self. and how long took for us to start to really review your > PR. > > Em qui, 23 de mai de 2019 às 12:36, Lucas Bonatto Miguel < > lucasbona...@gmail.com> escreveu: > > > Sounds good, I'll do that and I send the new PR link here soon. > > > > Best, > > Lucas > > > > On Thu, May 23, 2019 at 12:33 PM Wei Chen <weic...@apache.org> wrote: > > > > > One thing that I noticed is that: > > > If we go into the code and check the files. > > > They are still "executor". ex: EngineExecutorApp.scala > > > If we are going to change "executor" to "server", > > > we might need to modify the packages and most files. > > > > > > I think it will be simpler for us to revert PR#21 first. > > > Only make changes to include docker R support. (without changing name) > > > Make another PR about name changing and we can discuss there. > > > > > > Best Regards > > > Wei > > > > > > On Thu, May 23, 2019 at 1:21 PM Lucas Bonatto Miguel < > > lucasb...@apache.org > > > > > > > wrote: > > > > > > > Hey guys, in my last MR ( > > > > https://github.com/apache/incubator-marvin/pull/21) > > > > I added support for building docker images and also started > > implementing > > > > what we discussed as being the new architecture, that will have its > > roots > > > > on containers and REPL. > > > > > > > > During the work I realized that in the new context the > engine-executor > > > > would look more like an engine-server, given it's, in fact, a server > > for > > > > marvin engines implemented in any language. > > > > > > > > I think I underestimated the impact of this change, and now I > realized > > I > > > > will need to send some updates on the documentation. Other than that, > > > does > > > > anyone see any other thing that needs to be done? > > > > > > > > Best, > > > > Lucas > > > > > > > > > >
