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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
