>*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.
