I would agree on removing the partial deployment flag, but I do not like the all-or-nothing approach. I would still vote for the partial deployment support. Users should get an exception with services that failed and potentially retry them on their own.
Let's keep the partial deployment semantic on the deployAll(...) API. D. On Wed, Sep 6, 2017 at 9:40 AM, Vladimir Ozerov <[email protected]> wrote: > 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 <[email protected]> > 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 <[email protected]>: > > > > > 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 < > [email protected] > > > > > > wrote: > > > > > > > On Wed, Sep 6, 2017 at 8:24 AM, Pavel Tupitsyn <[email protected] > > > > > > 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 < > > [email protected]> > > > > > 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 < > > > [email protected]> > > > > > > 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 < > > > > > [email protected] > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > >> On Wed, Sep 6, 2017 at 8:10 AM, Vladimir Ozerov < > > > > [email protected] > > > > > > > > > > > > >> 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 < > > > > > > >> [email protected]> > > > > > > >> > 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 < > > > > > > [email protected] > > > > > > >> > > > > > > > >> > > 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 < > > > > > > >> > > [email protected]> > > > > > > >> > > > 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 < > > > > > > >> > > > [email protected]> > > > > > > >> > > > > 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 < > > > > > [email protected] > > > > > > >: > > > > > > >> > > > > > > > > > > > >> > > > > > > On Wed, Aug 16, 2017 at 8:17 AM, Denis Mekhanikov > < > > > > > > >> > > > > [email protected] > > > > > > >> > > > > > > > > > > > > >> > > > > > > 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 < > > > > > > >> > > > > [email protected] > > > > > > >> > > > > > >: > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > 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 > > > < > > > > > > >> > > > > > > [email protected] > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > 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 < > > > > > > >> > > > > > > [email protected] > > > > > > >> > > > > > > > >: > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > 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 > > > > > < > > > > > > >> > > > > > > > > [email protected] > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > 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 > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
