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 Mon, 24 Dec 2018 17:23:49 GMT
Igniters, especially future reviewers,

Discovery listener registered by 'IgniteServiceProcessor' become
implemented 'HighPriorityListener', seems it's best lock-free
solutions discussed during the review. This change is covered by
`ServiceDeploymentDiscoveryListenerNotificationOrderTest` which should
protect us if the order of listeners will be changed.

It's about the problem of custom messages which are nullified by PME
[1] and are listened by service deployment to manage the lifecycle of
affinity services. This guarantees that service deployment discovery
listener will be notified earlier than PME's discovery listener and
will be able to capture custom messages which may be nullified in PME
process.

Looks like we do not have any controversial questions now.

Thanks!

[1] http://apache-ignite-developers.2346864.n4.nabble.com/Danger-change-of-DiscoveryCustomEvent-in-GridDhtPartitionsExchangeFuture-onDone-td35946.html


On Mon, Dec 24, 2018 at 4:23 PM Vyacheslav Daradur <daradurvs@gmail.com> wrote:
>
> Stanislav, thank you for the notes, most of them have been resolved. I
> answered on GitHub.
>
>
> On Sun, Dec 23, 2018 at 9:34 PM Stanislav Lukyanov
> <stanlukyanov@gmail.com> wrote:
> >
> > I’ve done a quick superficial review. Didn’t look at the tests, didn’t dive
into the design, etc, just the code.
> > I’ve left some comments – almost all are about minor issues, grammar and code
style.
> >
> > Stan
> >
> > From: Vyacheslav Daradur
> > Sent: 21 декабря 2018 г. 14:58
> > To: dev@ignite.apache.org
> > Subject: Re: Service grid redesign
> >
> > 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.
> >
>
>
> --
> Best Regards, Vyacheslav D.



--
Best Regards, Vyacheslav D.

Mime
View raw message