Guys, If service deployment *can* be easily rolled back - then we do not need the flag and should have strict semantics "all-or-nothing". Users do not need partial semantics. This is not ATOMIC cache, this is services.
If service deployment *cannot* be rolled back - then we do not need the flag either, as we cannot guarantee atomicity and "all-or-nothing" simple cannot be implemented. In any case - flag is not needed. The question is only whether we choose "all-or-nothing" or "partial" approach. On Wed, Sep 6, 2017 at 6:58 PM, Denis Mekhanikov <dmekhani...@gmail.com> wrote: > > 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 > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > >
