> If we cannot rollback services, then what is the use of "boolean allOrNone"? Currently services deployment may fail only on configuration check or on write to the internal cache. Both of these operations are performed before any services are deployed, so rollback means just transaction rollback. In case if we decide to fix IGNITE-3392 <https://issues.apache.org/jira/browse/IGNITE-3392>, failure of initialization of some of the provided services will cause cancellation of all deployed services, so it's also a kind of a rollback.
I think, "allOrNone" mode is the most natural way to perform deployment, as I can't think of a use-case, when a partial deployment is an acceptable outcome. On the other hand, as Dmitriy noted, partial deployment with a proper exception may be useful for performing a "retry" for failed services. So, both of proposed modes may be used in different situations. - Denis ср, 6 сент. 2017 г. в 18:33, Vladimir Ozerov <voze...@gridgain.com>: > Dima, > > I agree with your reasoning. My outstanding question is why we have a flag? > If we cannot rollback services, then what is the use of "boolean > allOrNone"? Let's just remove it and always deploy services partially, > throwing Exception with proper infromation about failed services. > > On Wed, Sep 6, 2017 at 6:27 PM, Dmitriy Setrakyan <dsetrak...@apache.org> > wrote: > > > On Wed, Sep 6, 2017 at 8:24 AM, Pavel Tupitsyn <ptupit...@apache.org> > > wrote: > > > > > Agree with Vova, partial deployment does not make much sense in > deployAll > > > method. > > > Partial deployment can be performed with a deploy method in a loop. > > > > > > > That's exactly what we are trying to fix - deploy in a loop is slow and > > sequential. deployAll should be deploying services in parallel and > faster. > > > > > > We can certainly undeploy the services automatically, but it will require > > some additional code during the deployment for a very questionable value. > > > > > > > > > > On Wed, Sep 6, 2017 at 6:21 PM, Vladimir Ozerov <voze...@gridgain.com> > > > wrote: > > > > > > > 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 > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > >
