Slava,
I think, it's better to replace word "Change" with "Request".
Nik,
We have IgniteServiceProcessor and GridServiceProcessor with singular
"Service",
ServicesDeploymentManager and ServicesDeploymentTask with plural "Services"
for some reason.
So, you need to remember, where Service and where Services is used.
I think, we should unify these names.
And ServiceSingleDeploymentsResults name doesn't make sense to me.
"Single deployments" doesn't sound right.
ServicesFullDeploymentsMessage is derived
from GridDhtPartitionsFullMessage.
It doesn't really reflect its function. This message is supposed to mark
the point in time, when deployment is finished.
Denis
пт, 14 дек. 2018 г. в 11:30, Vyacheslav Daradur <daradu...@gmail.com>:
> >*1. Testing of the cache-based implementation of the service grid.*
> > I think, we should make a test suite, that will test the old
> implementation
> > until we remove it from the project.
>
> Agree. This is exactly what should be done as the first step once
> phase 1 will be merged.
> I think all tests in the package:
> "org.apache.ignite.internal.processors.service" should be moved to
> separate test-suite and new build-plan should be added on TC and
> included in RunAll.
>
> > *2. DynamicServiceChangeRequest.*
> > I think, this class should be splat into two.
>
> Personally, I agree, but I have faced opposition at the design step.
> I changed to the following structure:
>
> abstract class ServiceAbstractChange implements Serializable {
> protected final IgniteUuid srvcId;
> }
>
> class ServiceDeploymentChange extends ServiceAbstractChange {
> ServiceConfiguration cfg;
> }
>
> class ServiceUndeploymentChange extends ServiceAbstractChange { }
>
> I hope that further reviewers will agree with us.
>
> > *3. Naming.*
>
> About "Services" -> "Service" and "Deployments" -> "Deployment"
> Personally, I agree with Nikolay, because it's more descriptive since
> manages several services, not single.
> But, I understand Denis's point of view, we have a lot of classes with
> "Service" prefix in naming and "Services" looks a bit alien.
>
> > *DynamicServicesChangeRequestBatchMessage -> DynamicServiceChangeRequest*
> Prefix "Dynamic" has no sense anymore since we reworked message
> structure as in p.2. so "ServiceChangeBatchRequest" will be better
> name.
>
> > *ServicesSingleDeploymentsMessage -> ServiceDeploymentResponse*
> It's not a response and is not sent to the sender. This message is
> sent to the coordinator and contains *single node* deployments.
>
> > *ServicesFullDeploymentsMessage -> ServiceDeploymentFinishMessage*
> This should be named similar way as the previous one, but the message
> contains deployments of *full set of nodes*.
>
>
> On Fri, Dec 14, 2018 at 10:58 AM Nikolay Izhikov <nizhi...@apache.org>
> wrote:
> >
> > Hello, Denis.
> >
> > Great news.
> >
> > > *1. Testing of the cache-based implementation of the service grid.*
> > > I think, we should make a test suite, that will test the old
> implementation> until we> remove it from the project.
> >
> > Aggree. Let's do it.
> >
> > > *2. DynamicServiceChangeRequest.*
> > > I think, this class should be splat into two.
> >
> > Agree. Lets's do it.
> >
> > > *ServicesDeploymentManager*, *ServicesDeploymentTask *and all other
> classes> with Services word in them.
> > > I think, they would look better if we use a singular word *Service
> *instead.
> > > Same for *Deployments*.
> >
> > Personally, I want that names as clearly as possible reflects class
> content for reader.
> > If we deploy *several* services then it has to be Service*S*.
> >
> > Same for deployment - if this message will initiate single deployment
> process then it should use deployment.
> > otherwise - deployments.
> >
> > So my opinion - it's better to keep current naming.
> >
> > В Чт, 13/12/2018 в 19:36 +0300, Denis Mekhanikov пишет:
> > > Guys,
> > >
> > > I've been looking through the PR by Vyacheslav for past few weeks.
> > > Slava, great job! You've done an impressive amount of work.
> > >
> > > I posted my comments to the PR and had a few calls with Slava.
> > > I am close to finishing my review.
> > > There are some points, that I'd like to settle in this discussion to
> avoid
> > > controversy.
> > >
> > > *1. Testing of the cache-based implementation of the service grid.*
> > > I think, we should make a test suite, that will test the old
> implementation
> > > until we
> > > remove it from the project.
> > >
> > > *2. DynamicServiceChangeRequest.*
> > > I think, this class should be splat into two.
> > > I don't see any point in having a single class with "*flags"* field,
> that
> > > shows, what action it actually represents.
> > > Usage of *deploy(), markDeploy(...), undeploy(), markUndeploy(...)*
> looks
> > > wrong.
> > > Why not have a separate message type for each action instead?
> > >
> > > *3. Naming.*
> > > I suggest renaming the following classes:
> > > *ServicesDeploymentManager*, *ServicesDeploymentTask *and all other
> classes
> > > with Services word in them.
> > > I think, they would look better if we use a singular word *Service
> *instead.
> > > Same for *Deployments*.
> > > I propose the following class names:
> > >
> > > *ServicesDeploymentManager -> ServiceDeploymentManager*
> > > *ServicesDeploymentActions -> ServiceDeploymentActions*
> > > *ServicesDeploymentTask -> ServiceDeploymentTask*
> > > *ServicesCommonDiscoveryData -> ServiceCommonDiscoveryData*
> > > *ServicesJoinNodeDiscoveryData -> ServiceJoiningNodeDiscoveryData*
> > >
> > > *DynamicServicesChangeRequestBatchMessage ->
> DynamicServiceChangeRequest*
> > > *ServicesSingleDeploymentsMessage -> ServiceDeploymentResponse*
> > > *ServicesFullDeploymentsMessage -> ServiceDeploymentFinishMessage*
> > >
> > > *ServiceSingleDeploymentsResults -> ServiceSingleDeploymentResult*
> > > *ServiceFullDeploymentsResults -> ServiceFullDeploymentResult*
> > >
> > > Let's do this as the final step of the code review to avoid repeated
> > > renaming.
> > >
> > > Denis
> > >
> > > чт, 6 дек. 2018 г. в 15:21, Denis Mekhanikov <dmekhani...@gmail.com>:
> > >
> > > > Alexey,
> > > >
> > > > I don't see any problem in letting services work on a deactivated
> cluster.
> > > > All services need is discovery messages and compute tasks.
> > > > Both of these features are available at all times.
> > > >
> > > > But it should be configurable. Services may need caches for their
> work,
> > > > so it's better to undeploy such services on cluster deactivation.
> > > > We may introduce a new property in ServiceConfiguration.
> > > >
> > > > I think, this topic deserves a separate discussion.
> > > > Could you start another thread?
> > > >
> > > > Denis
> > > >
> > > > чт, 6 дек. 2018 г. в 13:27, Alexey Kuznetsov <akuznet...@apache.org
> >:
> > > >
> > > > > Hi, Vyacheslav!
> > > > >
> > > > > I'm thinking about to use Services API to implement Web Agent as a
> cluster
> > > > > singleton service.
> > > > > It will improve Web Console UX, because it will not needed to start
> > > > > separate java program.
> > > > > Just start cluster with Web agent enabled on cluster configuration.
> > > > >
> > > > > But in order to do this, I need that services should:
> > > > > 1) Work when cluster NOT ACTIVE.
> > > > > 2) Auto restart with cluster (when cluster was restarted).
> > > > >
> > > > > Could we support mentioned features on "Service Grid redesign -
> phase 2" ?
> > > > >
> > > > > Please let me know.
> > > > >
> > > > > --
> > > > > Alexey Kuznetsov
> > > > >
>
>
>
> --
> Best Regards, Vyacheslav D.
>
