ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vladimir Ozerov <voze...@gridgain.com>
Subject Re: Batch service deployment
Date Wed, 06 Sep 2017 15:10:12 GMT
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.

On Wed, Sep 6, 2017 at 6:06 PM, Dmitriy Setrakyan <dsetrakyan@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 <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