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