Well, if we cannot rollback services easily then *why* we have a mode where we declare a kind of false "atomicity"?
On Wed, Sep 6, 2017 at 6:21 PM, Vladimir Ozerov <voze...@gridgain.com> wrote: > Well, if we cannot rollback services easily then when we have a mode where > we declare a kind of false "atomicity"? > > On Wed, Sep 6, 2017 at 6:17 PM, Dmitriy Setrakyan <dsetrak...@apache.org> > wrote: > >> On Wed, Sep 6, 2017 at 8:10 AM, Vladimir Ozerov <voze...@gridgain.com> >> wrote: >> >> > Dima, >> > >> > No, my point is to remove method with flag and never allow partial >> > deployment. I do not needsee any practical use cases for this. >> > >> >> The problem is not in practical use cases, but also in our ability to >> rollback the already started services. I think it is much easier for us to >> support the partial deployment than try to implement complex rollback >> procedures. Also, from a user standpoint, it can be easily explained and >> seems to be a potentially useful feature. I would keep the partial >> deployment. >> >> >> > >> > On Wed, Sep 6, 2017 at 6:06 PM, Dmitriy Setrakyan < >> dsetrak...@apache.org> >> > wrote: >> > >> > > Vova, makes sense. Couple of comments. >> > > >> > > >> > > 1. allowPartialUpdate -> allowPartialDeploy >> > > 2. I do not think we need the 2nd deployAll method. This is not the >> > API >> > > where we need convenience shortcuts. >> > > 3. Partial deployment is a failure, not success, so the exception >> > should >> > > be thrown. However, we must make sure that this exception has list >> of >> > > services that failed to deploy with proper error messages, if >> > possible. >> > > >> > > D. >> > > >> > > On Wed, Sep 6, 2017 at 8:01 AM, Vladimir Ozerov <voze...@gridgain.com >> > >> > > wrote: >> > > >> > > > Igniters, >> > > > >> > > > Personally, I do not like the flag name - hard to understand and >> use. >> > > What >> > > > if instead we define the following API: >> > > > >> > > > void deployAll(Collection<ServiceConfiguration> cfgs, boolean >> > > > allowPartialUpdate) throws ServiceDeploymentException >> > > > void deployAll(Collection<ServiceConfiguration> cfgs) >> > > > throws ServiceDeploymentException >> > > > >> > > > The second method will delegate to deployAll(cfgs, false). This way >> in >> > > the >> > > > vast majority of cases user would not even bother about existence of >> > this >> > > > flag. >> > > > >> > > > But let's go deeper. If I allowed partial deployment and several >> > service >> > > > failed - is it success or failure? On the one hand, it is a kind of >> > > success >> > > > as I expected this, so I do not want exceptions. On the other hand >> this >> > > is >> > > > a kind of failure, so Exception might be ok. All this makes API >> hard to >> > > > reason about. Personally I do not understand why user may want to >> allow >> > > > partial registration in practice. We should allow only >> all-or-nothing >> > > mode. >> > > > And if something went wrong, we should return the list of offending >> > > > services in exception. This way API reduces to: >> > > > >> > > > void deployAll(Collection<ServiceConfiguration> cfgs) >> > > > throws ServiceDeploymentException >> > > > >> > > > Clean, simple, covers 99% of real use cases. >> > > > >> > > > Thoughts? >> > > > >> > > > >> > > > On Fri, Aug 18, 2017 at 4:16 AM, Dmitriy Setrakyan < >> > > dsetrak...@apache.org> >> > > > wrote: >> > > > >> > > > > Sounds good! Thanks for the detailed info. Can you please provide >> the >> > > > > updated API in the ticket? >> > > > > >> > > > > On Thu, Aug 17, 2017 at 12:41 AM, Denis Mekhanikov < >> > > > dmekhani...@gmail.com> >> > > > > wrote: >> > > > > >> > > > > > > Can we add an "allOrNone" flag to the deployment method? >> > > > > > >> > > > > > Sounds good, I think we can. >> > > > > > >> > > > > > > However, hot do you ensure atomicity here? >> > > > > > >> > > > > > We can guarantee that if some of configurations are invalid, or >> a >> > > > > > transaction, that writes configuration to the internal cache, >> > fails, >> > > > then >> > > > > > no services will be deployed. >> > > > > > >> > > > > > Currently we don't track failures on the server side and >> services >> > are >> > > > > > considered successfully deployed once their configurations are >> > > written >> > > > to >> > > > > > the cache. So, it's not possible that all configurations are >> valid, >> > > but >> > > > > > only a part of the services fail to deploy. If we change this >> > > behavior >> > > > > and >> > > > > > start tracking failures during deployment and initialization on >> the >> > > > > server, >> > > > > > then we could automatically cancel services that are already >> > deployed >> > > > in >> > > > > a >> > > > > > batch. >> > > > > > >> > > > > > чт, 17 авг. 2017 г. в 8:34, Dmitriy Setrakyan <d...@gridgain.com>: >> > > > > > >> > > > > > > On Wed, Aug 16, 2017 at 8:17 AM, Denis Mekhanikov < >> > > > > dmekhani...@gmail.com >> > > > > > > >> > > > > > > wrote: >> > > > > > > >> > > > > > > > I've had a few off-line conversations with other Igniters >> > > regarding >> > > > > > this >> > > > > > > > question and almost all of them think that services should >> be >> > > > > deployed >> > > > > > > with >> > > > > > > > "all-or-none" failing policy. >> > > > > > > > We have a similar functionality for caches: >> Ignite#createCaches >> > > > > method >> > > > > > > > don't allow partial deployments, and I think, we should also >> > > stick >> > > > to >> > > > > > > this >> > > > > > > > principle for services. >> > > > > > > > >> > > > > > > >> > > > > > > Can we add an "allOrNone" flag to the deployment method? If >> true, >> > > > then >> > > > > > all >> > > > > > > services will have to either be deployed or failed. However, >> hot >> > do >> > > > you >> > > > > > > ensure atomicity here? If you are deploying 10 services, and >> > only 1 >> > > > > > fails, >> > > > > > > what do you do with the other 9, given that they have already >> > been >> > > > > > deployed >> > > > > > > and may have started serving API requests? >> > > > > > > >> > > > > > > >> > > > > > > > >> > > > > > > > Another question that I'd like to discuss here is that >> > currently >> > > > > > > > IgniteServices#deployAsync method may fail with an exception >> > > > instead >> > > > > of >> > > > > > > > returning a future. Shouldn't we change this behavior to >> make >> > > async >> > > > > > > > operations always return a future whose get() method would >> > throw >> > > an >> > > > > > > > exception? >> > > > > > > > >> > > > > > > >> > > > > > > Makes sense to me. I think throwing exception from async >> method >> > is >> > > > > plain >> > > > > > > wrong. >> > > > > > > >> > > > > > > > >> > > > > > > > вт, 15 авг. 2017 г. в 11:42, Dmitriy Setrakyan < >> > > > > dsetrak...@apache.org >> > > > > > >: >> > > > > > > > >> > > > > > > > > Denis, >> > > > > > > > > >> > > > > > > > > I don't think we need a king deployment result. >> > > > > > > > > >> > > > > > > > > The "deployAllAsync" method should never throw an >> exception, >> > it >> > > > > > should >> > > > > > > > > always return the future. However, the >> IgniteFuture.get(...) >> > > > method >> > > > > > > does >> > > > > > > > > throw an exception, and in this exception you should >> provide >> > > the >> > > > > info >> > > > > > > > about >> > > > > > > > > the failures. >> > > > > > > > > >> > > > > > > > > D. >> > > > > > > > > >> > > > > > > > > On Tue, Aug 15, 2017 at 1:31 AM, Denis Mekhanikov < >> > > > > > > dmekhani...@gmail.com >> > > > > > > > > >> > > > > > > > > wrote: >> > > > > > > > > >> > > > > > > > > > Dmitriy, thank you for your reply! >> > > > > > > > > > >> > > > > > > > > > I see a possibility of a bad scenario here. If we use >> > > > > > deployAllAsync >> > > > > > > > > method >> > > > > > > > > > and it throws an exception, then the constructed future >> > won't >> > > > be >> > > > > > > > returned >> > > > > > > > > > and we won't have a way to wait for the rest of the >> > services >> > > to >> > > > > > > deploy. >> > > > > > > > > > Maybe we should return some king of deployment result, >> > > > > containing a >> > > > > > > > > future >> > > > > > > > > > along with a collection of failed services, instead of >> > > throwing >> > > > > an >> > > > > > > > > > exception? >> > > > > > > > > > >> > > > > > > > > > пн, 14 авг. 2017 г. в 18:03, Dmitriy Setrakyan < >> > > > > > > dsetrak...@apache.org >> > > > > > > > >: >> > > > > > > > > > >> > > > > > > > > > > Hi Denis, I agree, we should have an API for batch >> > service >> > > > > > > > deployment. >> > > > > > > > > My >> > > > > > > > > > > comments are inline... >> > > > > > > > > > > >> > > > > > > > > > > On Mon, Aug 14, 2017 at 2:22 AM, Denis Mekhanikov < >> > > > > > > > > dmekhani...@gmail.com >> > > > > > > > > > > >> > > > > > > > > > > wrote: >> > > > > > > > > > > >> > > > > > > > > > > > Hi Igniters! >> > > > > > > > > > > > >> > > > > > > > > > > > Currently Ignite doesn't have support for batch >> service >> > > > > > > deployment, >> > > > > > > > > but >> > > > > > > > > > > it >> > > > > > > > > > > > may be a very useful feature in case of a big >> number of >> > > > nodes >> > > > > > in >> > > > > > > a >> > > > > > > > > > > cluster >> > > > > > > > > > > > and services to be deployed. Each deployment >> includes >> > > write >> > > > > > into >> > > > > > > an >> > > > > > > > > > > > internal transactional cache, which is the longest >> part >> > > of >> > > > > the >> > > > > > > > > > procedure. >> > > > > > > > > > > > >> > > > > > > > > > > > I propose to optimize it by performing multiple >> writes >> > > in a >> > > > > > > single >> > > > > > > > > > > > transaction. It implies an introduction of a few new >> > > > methods >> > > > > in >> > > > > > > > > > > > IgniteServices interface. >> > > > > > > > > > > > I am thinking about the following signatures: >> > > > > > > > > > > > >> > > > > > > > > > > > void deployAll(Iterable<ServiceConfiguration> >> cfgs) >> > > > throws >> > > > > > > > > > > > IgniteException; >> > > > > > > > > > > > IgniteFuture<Void> >> > > > > > > deployAllAsync(Iterable<ServiceConfiguration> >> > > > > > > > > > > > cfgs) throws IgniteException; >> > > > > > > > > > > > >> > > > > > > > > > > > I'd like to know your opinion on the following >> > questions: >> > > > > > > > > > > > >> > > > > > > > > > > > - Do you agree with the proposed signatures? >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > Yes, but Iterable should be changed to Collection to >> be >> > > > > > consistent >> > > > > > > > with >> > > > > > > > > > > other similar APIs in Ignite. >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > - What should happen in case of a failure (some >> of >> > the >> > > > > > > > > > configurations >> > > > > > > > > > > > don't pass validation, or a service with >> specified >> > > name >> > > > > but >> > > > > > > > > > different >> > > > > > > > > > > > configuration already exists)? Should partial >> > > > deployments >> > > > > be >> > > > > > > > > > performed >> > > > > > > > > > > > in >> > > > > > > > > > > > case when some of them fail? >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > Yes, we should allow partial deployment. The exception >> > > thrown >> > > > > > > should >> > > > > > > > > > have a >> > > > > > > > > > > collection of services that have failed deployment. It >> > > looks >> > > > > like >> > > > > > > you >> > > > > > > > > > will >> > > > > > > > > > > need to create ServiceDeploymentException (extends >> > > > > > IgniteException) >> > > > > > > > to >> > > > > > > > > > > handle this case (in which case, you have to make sure >> > that >> > > > > other >> > > > > > > > > deploy >> > > > > > > > > > > methods also throw it). >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > Regarding the second question I think that we >> shouldn't >> > > > > deploy >> > > > > > > any >> > > > > > > > > > > services >> > > > > > > > > > > > in a batch if we encounter any problems with some of >> > > them. >> > > > > > > > > > > > >> > > > > > > > > > > > Also cancelAll method may be optimized in a similar >> > way, >> > > > but >> > > > > no >> > > > > > > > > > interface >> > > > > > > > > > > > changes are needed there. >> > > > > > > > > > > > >> > > > > > > > > > > > Ticket: https://issues.apache.org/ >> > > jira/browse/IGNITE-5145 >> > > > > > > > > > > > >> > > > > > > > > > > > -- >> > > > > > > > > > > > Cheers, >> > > > > > > > > > > > Denis Mekhanikov >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >
