You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@marvin.apache.org by Lucas Bonatto Miguel <lu...@apache.org> on 2019/05/23 18:21:09 UTC
engine-executor renamed
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
Re: engine-executor renamed
Posted by Daniel Takabayashi <da...@gmail.com>.
Hi Huys, sorry to give my comments so late but...
I suppose Lucas is talking about these changes over [here|
https://github.com/apache/incubator-marvin/pull/21]
I think our primer rule should avoid changing on the existent structure
right now, once we don't have too many contributors(like change the
engine-executor to engine-server), and this is why I think this:
- Internal link, names, and scripts must be refactored, and this means
more efforts to test and guaranty that everything is working
- External link, as articles, manuals, citations, repositories (pip,
maven) should also be changed otherwise we going to create a bunch of
broken links
- package names should be changed to follow good practices
We need to think about what are the gains of changing the sub-project name.
Til now, my perspective is, doesn't worth it.
Personally, I don't agree we need to do this. I think to be a little safer
we just should create the new staffs and refactor the minimum to avoid
significant efforts and long reviews.
Just to help us to remember ... this was the architectural we decide to
implement.
marvin-architecture-views.svg
<https://issues.apache.org/jira/secure/attachment/12969536/12969536_marvin-architecture-views.svg>
My opinion, in this case, let's roll back the commit and then Lucas you
should just open a new PR changing the code related with the task he is
doing and following the decided architecture reference.
One way to do this could be following these tasks:
1 - Create a new project called *marvin-toolbox* with one CLI command (
*generate-env*) that using a component called *docker-client* to
create/provisioning a docker image based are public examples (
https://github.com/apache/incubator-marvin/tree/develop/public-engines/iris-species-engine
).
2 - Change the recent create project *marvin-toolbox* creating new commands
to wrapper all the commands the engines already have (start, dryrun, test,
etc)
3 - Create an empty engine template (*python-engine-template*) to be used
to create new engines
4 - Change the recent create project *marvin-toolbox *creating a new
command call (generate or init) to allow users to create new engines based
on the *python-engine-template*
5 - Create an empty engine template (*r-engine-template*) to be used to
create new engines
6 - Allow the generate-env and generate CLI commands inside
*marvin-toolbox *to create engine based on the *r-engine-template*
...
Em qui, 23 de mai de 2019 às 11:21, Lucas Bonatto Miguel <
lucasbm88@apache.org> escreveu:
> 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
>
Re: PR#21 Actions
Posted by Wei Chen <we...@apache.org>.
Hello Lucas,
PR merged, please feel free to submit a new PR for executor rename.
Best Regards
Wei
On Wed, May 29, 2019 at 12:24 AM Wei Chen <we...@apache.org> wrote:
> Cool!
> Just a quick note to everyone:
> https://github.com/apache/incubator-marvin/pull/23
> Keep the changes for Docker R Init.
> This should make things easier.
>
> Best Regards
> Wei
>
> On Wed, May 29, 2019 at 12:15 AM Lucas Bonatto Miguel <
> lucasbm88@apache.org> wrote:
>
>> Sorry for the delay on that. This PR reverts the name changes to give a
>> new
>> opportunity to the community to discuss what's the name that the module
>> executor should have in the new architecture.
>>
>> https://issues.apache.org/jira/browse/MARVIN-48
>>
>> Regards,
>> Lucas
>>
>> On Wed, May 29, 2019 at 1:50 AM Wei Chen <we...@apache.org> wrote:
>>
>> > Hello Lucas,
>> >
>> > Can you help to raise the revert PR for PR#21?
>> > And please help to submit 2 new PRs for Docker R Init and Engine Rename
>> > again.
>> > We can merge the Docker R Init PR asap.
>> >
>> > Best Regards
>> > Wei
>> >
>>
>
Re: PR#21 Actions
Posted by Wei Chen <we...@apache.org>.
Cool!
Just a quick note to everyone:
https://github.com/apache/incubator-marvin/pull/23
Keep the changes for Docker R Init.
This should make things easier.
Best Regards
Wei
On Wed, May 29, 2019 at 12:15 AM Lucas Bonatto Miguel <lu...@apache.org>
wrote:
> Sorry for the delay on that. This PR reverts the name changes to give a new
> opportunity to the community to discuss what's the name that the module
> executor should have in the new architecture.
>
> https://issues.apache.org/jira/browse/MARVIN-48
>
> Regards,
> Lucas
>
> On Wed, May 29, 2019 at 1:50 AM Wei Chen <we...@apache.org> wrote:
>
> > Hello Lucas,
> >
> > Can you help to raise the revert PR for PR#21?
> > And please help to submit 2 new PRs for Docker R Init and Engine Rename
> > again.
> > We can merge the Docker R Init PR asap.
> >
> > Best Regards
> > Wei
> >
>
Re: PR#21 Actions
Posted by Daniel Takabayashi <da...@gmail.com>.
+1
Em qua, 29 de mai de 2019 às 15:31, Wei Chen <we...@apache.org> escreveu:
> Got to agree with Taka.
> I admit that I merged it too quickly because I want to modify the Scala
> package name.
> I will just revert the history of those unchanged files together with
> PR#24.
> But keep the history of those files with actual changes.
>
> Does that sound fair to you Taka?
>
> Best Regards
> Wei
>
>
> On Wed, May 29, 2019 at 5:00 PM Daniel Takabayashi <
> daniel.takabayashi@gmail.com> wrote:
>
> > Hi,
> >
> > I believe we should revert the PR#21 instead of just rename the files
> back.
> > It doesn't make sense to me, once in the previous PR#21 he changed 91
> files
> > and now he is changing back 90 to the original version. This will
> generate
> > 180 fake changes in our repository. It is much easier and clean if we
> just
> > revert the PR#21 and merge the new PR#23 with only the new changes.
> >
> > Em ter, 28 de mai de 2019 às 22:15, Lucas Bonatto Miguel <
> > lucasbm88@apache.org> escreveu:
> >
> > > Sorry for the delay on that. This PR reverts the name changes to give a
> > new
> > > opportunity to the community to discuss what's the name that the module
> > > executor should have in the new architecture.
> > >
> > > https://issues.apache.org/jira/browse/MARVIN-48
> > >
> > > Regards,
> > > Lucas
> > >
> > > On Wed, May 29, 2019 at 1:50 AM Wei Chen <we...@apache.org> wrote:
> > >
> > > > Hello Lucas,
> > > >
> > > > Can you help to raise the revert PR for PR#21?
> > > > And please help to submit 2 new PRs for Docker R Init and Engine
> Rename
> > > > again.
> > > > We can merge the Docker R Init PR asap.
> > > >
> > > > Best Regards
> > > > Wei
> > > >
> > >
> >
>
Re: PR#21 Actions
Posted by Wei Chen <we...@apache.org>.
Got to agree with Taka.
I admit that I merged it too quickly because I want to modify the Scala
package name.
I will just revert the history of those unchanged files together with PR#24.
But keep the history of those files with actual changes.
Does that sound fair to you Taka?
Best Regards
Wei
On Wed, May 29, 2019 at 5:00 PM Daniel Takabayashi <
daniel.takabayashi@gmail.com> wrote:
> Hi,
>
> I believe we should revert the PR#21 instead of just rename the files back.
> It doesn't make sense to me, once in the previous PR#21 he changed 91 files
> and now he is changing back 90 to the original version. This will generate
> 180 fake changes in our repository. It is much easier and clean if we just
> revert the PR#21 and merge the new PR#23 with only the new changes.
>
> Em ter, 28 de mai de 2019 às 22:15, Lucas Bonatto Miguel <
> lucasbm88@apache.org> escreveu:
>
> > Sorry for the delay on that. This PR reverts the name changes to give a
> new
> > opportunity to the community to discuss what's the name that the module
> > executor should have in the new architecture.
> >
> > https://issues.apache.org/jira/browse/MARVIN-48
> >
> > Regards,
> > Lucas
> >
> > On Wed, May 29, 2019 at 1:50 AM Wei Chen <we...@apache.org> wrote:
> >
> > > Hello Lucas,
> > >
> > > Can you help to raise the revert PR for PR#21?
> > > And please help to submit 2 new PRs for Docker R Init and Engine Rename
> > > again.
> > > We can merge the Docker R Init PR asap.
> > >
> > > Best Regards
> > > Wei
> > >
> >
>
Re: PR#21 Actions
Posted by Daniel Takabayashi <da...@gmail.com>.
Hi,
I believe we should revert the PR#21 instead of just rename the files back.
It doesn't make sense to me, once in the previous PR#21 he changed 91 files
and now he is changing back 90 to the original version. This will generate
180 fake changes in our repository. It is much easier and clean if we just
revert the PR#21 and merge the new PR#23 with only the new changes.
Em ter, 28 de mai de 2019 às 22:15, Lucas Bonatto Miguel <
lucasbm88@apache.org> escreveu:
> Sorry for the delay on that. This PR reverts the name changes to give a new
> opportunity to the community to discuss what's the name that the module
> executor should have in the new architecture.
>
> https://issues.apache.org/jira/browse/MARVIN-48
>
> Regards,
> Lucas
>
> On Wed, May 29, 2019 at 1:50 AM Wei Chen <we...@apache.org> wrote:
>
> > Hello Lucas,
> >
> > Can you help to raise the revert PR for PR#21?
> > And please help to submit 2 new PRs for Docker R Init and Engine Rename
> > again.
> > We can merge the Docker R Init PR asap.
> >
> > Best Regards
> > Wei
> >
>
Re: PR#21 Actions
Posted by Lucas Bonatto Miguel <lu...@apache.org>.
Sorry for the delay on that. This PR reverts the name changes to give a new
opportunity to the community to discuss what's the name that the module
executor should have in the new architecture.
https://issues.apache.org/jira/browse/MARVIN-48
Regards,
Lucas
On Wed, May 29, 2019 at 1:50 AM Wei Chen <we...@apache.org> wrote:
> Hello Lucas,
>
> Can you help to raise the revert PR for PR#21?
> And please help to submit 2 new PRs for Docker R Init and Engine Rename
> again.
> We can merge the Docker R Init PR asap.
>
> Best Regards
> Wei
>
PR#21 Actions
Posted by Wei Chen <we...@apache.org>.
Hello Lucas,
Can you help to raise the revert PR for PR#21?
And please help to submit 2 new PRs for Docker R Init and Engine Rename
again.
We can merge the Docker R Init PR asap.
Best Regards
Wei
Re: engine-executor renamed
Posted by Wei Chen <we...@apache.org>.
Currently, we agree on the docker R engine init support for sure and I
think no one is against it.
I don't have a specific opinion about the naming part,
but I do believe we need more modifications for those filenames and package
names if we decided to change it.
+0 for naming change
I don't think we need to vote on the other parts (docker R init) either.
but I can give my +1 on those if counted.
Let's quickly go through the process and push the project forward.
Best Regards
Wei
On Fri, May 24, 2019 at 8:27 AM Daniel Takabayashi <
daniel.takabayashi@gmail.com> wrote:
> Lucas, to avoid conflicts the Apache rules allow us to call a vote session.
>
> https://httpd.apache.org/dev/guidelines.html
>
> Please just vote.
>
> Thanks,
> Taka
>
> Sent from my iPhone
>
> > On May 23, 2019, at 1:40 PM, Lucas Bonatto Miguel <
> lucasbonatto@gmail.com> wrote:
> >
> > 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.takabayashi@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 <
> >> lucasbonatto@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 <we...@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 <
> >>> lucasbm88@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
> >>>>>
> >>>>
> >>>
> >>
>
Re: engine-executor renamed
Posted by Daniel Takabayashi <da...@gmail.com>.
Lucas, to avoid conflicts the Apache rules allow us to call a vote session.
https://httpd.apache.org/dev/guidelines.html
Please just vote.
Thanks,
Taka
Sent from my iPhone
> On May 23, 2019, at 1:40 PM, Lucas Bonatto Miguel <lu...@gmail.com> wrote:
>
> 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.takabayashi@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 <
>> lucasbonatto@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 <we...@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 <
>>> lucasbm88@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
>>>>>
>>>>
>>>
>>
Re: engine-executor renamed
Posted by Lucas Bonatto Miguel <lu...@gmail.com>.
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.takabayashi@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 <
> lucasbonatto@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 <we...@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 <
> > lucasbm88@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
> > > >
> > >
> >
>
Re: engine-executor renamed
Posted by Daniel Takabayashi <da...@gmail.com>.
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 <
lucasbonatto@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 <we...@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 <
> lucasbm88@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
> > >
> >
>
Re: engine-executor renamed
Posted by Lucas Bonatto Miguel <lu...@gmail.com>.
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 <we...@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 <lucasbm88@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
> >
>
Re: engine-executor renamed
Posted by Wei Chen <we...@apache.org>.
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 <lu...@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
>