ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nikolay Izhikov <nizhi...@apache.org>
Subject Re: Service grid redesign
Date Thu, 20 Dec 2018 05:58:18 GMT
Denis, great news!

Alexey, Vova, Yakov, do you want to take a look at this PR?



В Ср, 19/12/2018 в 18:47 +0300, Denis Mekhanikov пишет:
> Guys,
> 
> I finished my code review. The pool request looks good to me.
> 
> Does anybody else want to look at the changes?
> There are a few points, that we didn't meet an agreement on,
> though they don't affect the behaviour in any way:
> 
>    - *Class naming. * See the discussion above.
>    - *Unnecessary task object cleaning. *
>    IMO, ServicesDeploymentTask#clear() method doesn't do anything useful,
>    and it should be removed.
>    By the moment, when this method is called, the task object is removed
>    from all collections anyway, so it's ready for garbage collection.
>    Removing data from it doesn't help anybody.
>    -
> *Unnecessary tests. *ServiceInfoSelfTest and
>    ServicesDeploymentProcessIdSelfTest look excessive to me.
>    I don't see any point in testing an interface implementation, that only
>    saves some objects and returns them from certain methods.
>    - Interface for events with servicesDeploymentActions() method.
>    Take a look at the discussion:
>    https://github.com/apache/ignite/pull/4434/files/30e69d9a53ce6ea16c4e9d15354e94360caa719d#r239442342
> 
> Also solution with *DiscoveryCustomEvent#nullifyingCustomMsgLock* looks
> clumsy to me.
> The problem with nullifying of *DiscoveryCustomEvent#customMsg* field can
> be solved
> by making *ServiceDiscoveryListener* a high priority listener.
> 
> Or *DiscoveryCustomEvent#customMessage()* method could be marked
> synchronized and
> *GridEventStorageManager#notifyListeners(..)* method could synchronize on
> the event object.
> But this solution is the same, it's just a matter of taste.
> 
> If anybody wants to look the the code of the PR, please consider these
> points as well.
> 
> Denis
> 
> ср, 19 дек. 2018 г. в 17:37, Nikolay Izhikov <nizhikov@apache.org>:
> 
> > Denis,
> > 
> > I don't think that differences with your and my naming is huge :)
> > And, it's definetely a matter of taste.
> > 
> > If there is no any other issues with PR let's rename and move on! :)
> > 
> > ср, 19 дек. 2018 г. в 17:32, Vyacheslav Daradur <daradurvs@gmail.com>:
> > 
> > > > We have IgniteServiceProcessor and GridServiceProcessor with singular
> > > 
> > > "Service"
> > > 
> > > Maybe we should rename new 'IgniteServiceProcessor' to
> > > 'IgniteServicesProcessor'?
> > > 
> > > > And ServiceSingleDeploymentsResults name doesn't make sense to me.
> > > > "Single deployments" doesn't sound right.
> > > 
> > > 'Single' means 'single node', maybe we should use one of the following:
> > > - 'ServicesSingleNodeDeploymentsResults'
> > > - 'ServicesNodeDeploymentsResults'
> > > - 'ServicesInstanceDeploymentsResults'
> > > 
> > > On Wed, Dec 19, 2018 at 4:26 PM Denis Mekhanikov <dmekhanikov@gmail.com>
> > > wrote:
> > > > 
> > > > 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 <daradurvs@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 <
> > 
> > nizhikov@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 <
> > > 
> > > dmekhanikov@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
<
> > > 
> > > akuznetsov@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.
> > > > > 
> > > 
> > > 
> > > 
> > > --
> > > Best Regards, Vyacheslav D.
> > > 

Mime
View raw message