ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vyacheslav Daradur <daradu...@gmail.com>
Subject Re: Service grid redesign
Date Fri, 21 Dec 2018 11:57:49 GMT
Igniters,

Please, let us know if someone is going to do an additional review?

We should know can we merge the PR since it has been approved by
Nikolay Izhikov and Denis Mekhanikov or we should wait for other
community members.

On Thu, Dec 20, 2018 at 7:52 PM Vyacheslav Daradur <daradurvs@gmail.com> wrote:
>
> I think I found names which should satisfy me and Denis, and possibly Nikolay )
>
> See the following names (Actual name <- Previously used):
>
> - ServiceDeploymentManager <- ServicesDeploymentManager
> - ServiceDeploymentActions <- ServicesDeploymentActions
> - ServiceDeploymentProcessId <- ServicesDeploymentProcessId
> - ServiceDeploymentTask <- ServicesDeploymentTask
>
> - ServiceDeploymentRequest <- ServiceDeploymentChange
> - ServiceUndeploymentRequest <- ServiceUndeploymentChange
> - ServiceChangeAbstractRequest <- ServiceAbstractChange
>
> - ServiceSingleNodeDeploymentResult <- ServiceSingleDeploymentsResults
> - ServiceSingleNodeDeploymentResultBatch <- ServicesSingleDeploymentsMessage
>
> - ServiceClusterDeploymentResult <- ServiceFullDeploymentsResults
> - ServiceClusterDeploymentResultBatch <- ServicesFullDeploymentsMessage
>
> - ServiceProcessorCommonDiscoveryData <- ServicesCommonDiscoveryData
> - ServiceProcessorJoinNodeDiscoveryData <- ServicesJoinNodeDiscoveryData
>
> Also, I had a short talk with Alexey Goncharuk about the problem of
> nullified custom messages. I changed the implementation to a lock-free
> solution which allows us to nullify messages depend on an using
> counter.
>
> In comparison with high priority listener, this allows us to not copy
> custom discovery event in service deployment manager and work with the
> original object.
>
> On Thu, Dec 20, 2018 at 8:57 AM Nikolay Izhikov <nizhikov@apache.org> wrote:
> >
> > 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.
> > > > >
>
>
>
> --
> Best Regards, Vyacheslav D.



-- 
Best Regards, Vyacheslav D.

Mime
View raw message