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 < [email protected]> 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 <[email protected]> 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 < > [email protected] > > > > > 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 > > > > > >
