>*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 <[hidden email]> 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 <[hidden email]>: > > > > > 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 <[hidden email]>: > > > > > > > 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. |
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 <[hidden email]>: > >*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 <[hidden email]> > 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 <[hidden email]>: > > > > > > > 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 <[hidden email] > >: > > > > > > > > > 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. > |
> 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 <[hidden email]> 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 <[hidden email]>: > > > >*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 <[hidden email]> > > 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 <[hidden email]>: > > > > > > > > > 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 <[hidden email] > > >: > > > > > > > > > > > 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. |
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 <[hidden email]>: > > 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 <[hidden email]> > 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 <[hidden email]>: > > > > > >*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 <[hidden email]> > > > 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 < > [hidden email]>: > > > > > > > > > > > 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 < > [hidden email] > > > >: > > > > > > > > > > > > > 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. > |
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 <[hidden email]>: > 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 <[hidden email]>: > > > > 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 <[hidden email]> > > 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 <[hidden email]>: > > > > > > > >*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 < > [hidden email]> > > > > 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 < > > [hidden email]>: > > > > > > > > > > > > > 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 < > > [hidden email] > > > > >: > > > > > > > > > > > > > > > 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. > > > |
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 <[hidden email]>: > > > 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 <[hidden email]>: > > > > > > 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 <[hidden email]> > > > 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 <[hidden email]>: > > > > > > > > > > *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 < > > > > [hidden email]> > > > > > 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 < > > > > > > [hidden email]>: > > > > > > > > > > > > > > > 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 < > > > > > > [hidden email] > > > > > > : > > > > > > > > > > > > > > > > > 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. > > > |
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 <[hidden email]> 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 <[hidden email]>: > > > > > 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 <[hidden email]>: > > > > > > > > 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 <[hidden email]> > > > > 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 <[hidden email]>: > > > > > > > > > > > > *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 < > > > > > > [hidden email]> > > > > > > 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 < > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > 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 < > > > > > > > > [hidden email] > > > > > > > : > > > > > > > > > > > > > > > > > > > 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. |
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > 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 <[hidden email]>: > > > > > > > > > > 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 <[hidden email]> > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > *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 < > > > > > > > > [hidden email]> > > > > > > > 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 < > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > [hidden email] > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > 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. |
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: [hidden email] 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > 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 <[hidden email]>: > > > > > > > > > > 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 <[hidden email]> > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > *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 < > > > > > > > > [hidden email]> > > > > > > > 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 < > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > [hidden email] > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > 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. |
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 <[hidden email]> 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: [hidden email] > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > 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 <[hidden email]> > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > *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 < > > > > > > > > > > [hidden email]> > > > > > > > > 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 < > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > [hidden email] > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > 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. |
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 <[hidden email]> 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 > <[hidden email]> 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: [hidden email] > > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > 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 <[hidden email]> > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > *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 < > > > > > > > > > > > > [hidden email]> > > > > > > > > > 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 < > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > 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. |
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 <[hidden email]> 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 > > <[hidden email]> 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: [hidden email] > > > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > 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 <[hidden email]> > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > > *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 < > > > > > > > > > > > > > > [hidden email]> > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. |
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 <[hidden email]> 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 > > > <[hidden email]> 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: [hidden email] > > > > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > 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 <[hidden email]> > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > > > > *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 < > > > > > > > > > > > > > > > > [hidden email]> > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. |
Igniters,
to have an opportunity to test both new and old service grid implementations, we added new config-plans on TC: - Service Grid [1] - runs tests in new (default) mode - Service Grid (legacy mode) [2] - runs tests in old (legacy) mode Both plans contain the following test-suites: - IgniteServiceGridTestSuite (newly added suite) - IgniteServiceConfigVariationsFullApiTestSuite (was moved from "Basic 2") Also, the plans have been included in RunAll. [1] https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGrid [2] https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGridLegacyMode On Thu, Dec 27, 2018 at 6:24 AM Nikolay Izhikov <[hidden email]> wrote: > > 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 <[hidden email]> 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 > > > > <[hidden email]> 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: [hidden email] > > > > > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > > 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 <[hidden email]> > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > *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 < > > > > > > > > > > > > > > > > > > [hidden email]> > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. -- Best Regards, Vyacheslav D. |
The wiki's article [1] has been updated according to the merged solution [2].
[1] https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95654584 [2] https://issues.apache.org/jira/browse/IGNITE-9607 On Fri, Dec 28, 2018 at 1:10 PM Vyacheslav Daradur <[hidden email]> wrote: > > Igniters, > > to have an opportunity to test both new and old service grid > implementations, we added new config-plans on TC: > - Service Grid [1] - runs tests in new (default) mode > - Service Grid (legacy mode) [2] - runs tests in old (legacy) mode > > Both plans contain the following test-suites: > - IgniteServiceGridTestSuite (newly added suite) > - IgniteServiceConfigVariationsFullApiTestSuite (was moved from "Basic 2") > > Also, the plans have been included in RunAll. > > [1] https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGrid > [2] https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGridLegacyMode > > On Thu, Dec 27, 2018 at 6:24 AM Nikolay Izhikov <[hidden email]> wrote: > > > > 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 <[hidden email]> 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 > > > > > <[hidden email]> 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: [hidden email] > > > > > > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > > > > 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 <[hidden email]> > > > > > > > > > > > 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > *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 < > > > > > > > > > > > > > > > > > > > > [hidden email]> > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > -- > Best Regards, Vyacheslav D. -- Best Regards, Vyacheslav D. |
Vyacheslav,
Let me speak for the community and Ignite users and thank you for achieving this milestone! Seems that the foundation for much bigger and impactful improvements is ready. Tremendous job. Are you going to start working on the services hot redeployment next? - Denis On Wed, Jan 16, 2019 at 1:36 AM Vyacheslav Daradur <[hidden email]> wrote: > The wiki's article [1] has been updated according to the merged solution > [2]. > > [1] > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95654584 > [2] https://issues.apache.org/jira/browse/IGNITE-9607 > > On Fri, Dec 28, 2018 at 1:10 PM Vyacheslav Daradur <[hidden email]> > wrote: > > > > Igniters, > > > > to have an opportunity to test both new and old service grid > > implementations, we added new config-plans on TC: > > - Service Grid [1] - runs tests in new (default) mode > > - Service Grid (legacy mode) [2] - runs tests in old (legacy) mode > > > > Both plans contain the following test-suites: > > - IgniteServiceGridTestSuite (newly added suite) > > - IgniteServiceConfigVariationsFullApiTestSuite (was moved from "Basic > 2") > > > > Also, the plans have been included in RunAll. > > > > [1] > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGrid > > [2] > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGridLegacyMode > > > > On Thu, Dec 27, 2018 at 6:24 AM Nikolay Izhikov <[hidden email]> > wrote: > > > > > > 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 < > [hidden email]> 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 > > > > > > <[hidden email]> 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: [hidden email] > > > > > > > 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 < > [hidden email]> 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 < > [hidden email]> 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 < > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > 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 < > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > 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 < > [hidden email]> > > > > > > > > > > > > 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 < > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > *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 > < > > > > > > > > > > > > > > > > > > > > > > [hidden email]> > > > > > > > > > > > > > > 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 > < > > > > > > > > > > > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > -- > > Best Regards, Vyacheslav D. > > > > -- > Best Regards, Vyacheslav D. > |
Denis, thank you! I’m glad to participate in development of the project.
Unfortunately, we don’t have clear design of the services hot redeployment yet. I’m going to start discussions about Service Grid roadmap and services hot redevelopment design in separate threads within a couple of weeks. пт, 18 янв. 2019 г. в 0:06, Denis Magda <[hidden email]>: > Vyacheslav, > > Let me speak for the community and Ignite users and thank you for achieving > this milestone! Seems that the foundation for much bigger and impactful > improvements is ready. Tremendous job. > > Are you going to start working on the services hot redeployment next? > > > - > Denis > > > On Wed, Jan 16, 2019 at 1:36 AM Vyacheslav Daradur <[hidden email]> > wrote: > > > The wiki's article [1] has been updated according to the merged solution > > [2]. > > > > [1] > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95654584 > > [2] https://issues.apache.org/jira/browse/IGNITE-9607 > > > > On Fri, Dec 28, 2018 at 1:10 PM Vyacheslav Daradur <[hidden email]> > > wrote: > > > > > > Igniters, > > > > > > to have an opportunity to test both new and old service grid > > > implementations, we added new config-plans on TC: > > > - Service Grid [1] - runs tests in new (default) mode > > > - Service Grid (legacy mode) [2] - runs tests in old (legacy) mode > > > > > > Both plans contain the following test-suites: > > > - IgniteServiceGridTestSuite (newly added suite) > > > - IgniteServiceConfigVariationsFullApiTestSuite (was moved from "Basic > > 2") > > > > > > Also, the plans have been included in RunAll. > > > > > > [1] > > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGrid > > > [2] > > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ServiceGridLegacyMode > > > > > > On Thu, Dec 27, 2018 at 6:24 AM Nikolay Izhikov <[hidden email]> > > wrote: > > > > > > > > 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 < > > [hidden email]> 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 > > > > > > > <[hidden email]> 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: [hidden email] > > > > > > > > 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 < > > [hidden email]> 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 < > > [hidden email]> 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 < > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > 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 < > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > [hidden email]> > > > > > > > > > > > > > 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 < > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > *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 > > < > > > > > > > > > > > > > > > > > > > > > > > > [hidden email]> > > > > > > > > > > > > > > > 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 > > < > > > > > > > > > > > > > > > > > > > > > > > > > > [hidden email]>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > > > > > > > > > > > > > > > [hidden email] > > > > > > > > > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > -- > > > Best Regards, Vyacheslav D. > > > > > > > > -- > > Best Regards, Vyacheslav D. > > > Best Regards, Vyacheslav D. |
Free forum by Nabble | Edit this page |