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 <stanlukya...@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 <daradu...@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 <nizhi...@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 <nizhi...@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 <daradu...@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 > > > > > > <dmekhani...@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 > > > > > > > <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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > -- > > Best Regards, Vyacheslav D. > > > > -- > Best Regards, Vyacheslav D. > -- Best Regards, Vyacheslav D.
