Hello, Igniters. I've merged Service Grid Redesign - Phase 1 to the master. Vyacheslav, great contribution!
Thanks for all Ignite veterans both for the code and design review. В Пн, 24/12/2018 в 20:50 +0300, Nikolay Izhikov пишет: > Hello, Igniters. > > Please, let us know, if someone want to do additional review of this PR. > > В Пн, 24/12/2018 в 20:23 +0300, Vyacheslav Daradur пишет: > > 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 <daradu...@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 > > > <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. > > > > > > > > -- > > Best Regards, Vyacheslav D.
signature.asc
Description: This is a digitally signed message part
