ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: Batch service deployment
Date Wed, 06 Sep 2017 15:06:14 GMT
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 <vozerov@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 <dsetrakyan@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 <
> dmekhanikov@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 <
> > dmekhanikov@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 <
> > dsetrakyan@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 <
> > > > dmekhanikov@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 <
> > > > dsetrakyan@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
<
> > > > > > dmekhanikov@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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message