openwhisk-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rodric Rabbah <rod...@gmail.com>
Subject Re: ArtifactStore shutdown handling and shared resources
Date Tue, 26 Jun 2018 13:18:50 GMT
+1 to refcount, i think it's better to shut down the pool then possibly
leave open connections that may cause trouble when you replace a controller
in production.
the implementation seems straightforward here.

-r

On Tue, Jun 26, 2018 at 1:40 AM, Chetan Mehrotra <chetan.mehrotra@gmail.com>
wrote:

> > it hasn’t been clear that this is a problem (other than good citizenship
> dictating that you should gracefully close resources)
>
> Yes its more about implementing the contract of ArtifactStore
> interface properly. For now if not done properly it should not cause
> much harm barring test which I would need to check
>
> > These would connect to the same endpoint, sure, but is that a problem
> other than inflating the connection pool slightly? It might make things
> more consistent with other artifact stores, with the sacrifice of some
> azure purity that not have real impact.
>
> I would prefer to reuse the connection thread pool as given how
> queries in CosmosDB work I expect lots of connections to be used
> concurrently. So better to reuse the pool across 3 collections
>
> > If there is significant problem with not revising the SPI to better
> handle shutdown (or other aspects), an alternate approach would be to
> address the SPI first.
>
> How SPI lifecyle is managed would need to be discussed. In other
> places often some IoC container gets used which manages the lifecycle
> aspect in a std manner. Given our current SPI model we would need to
> handle that.
>
> It would be better to address the shutdown semantics in a separate
> issue as its orthogonal to current PR objective.
>
> For this PR we have 2 option (unless less someone can suggest an
> alternative)
>
> 1. Accept current state of rely on ref counting
> 2. Do not close the pool on shutdown with assumption it should not
> cause much issue
>
> Chetan Mehrotra
>
> On Thu, Jun 21, 2018 at 9:06 PM, Tyson Norris <tnorris@adobe.com.invalid>
> wrote:
> >
> > I prefer not to introduce the ref counting - it is cryptic and if it is
> known to need replacement, these are signs that it should not go in.
> >
> > What is the exact affect treating shutdown the same as in couchdb store
> for now? (Meaning, shutdown is not really handled explicitly)
> > Other than possibly complicating test scenarios, it hasn’t been clear
> that this is a problem (other than good citizenship dictating that you
> should gracefully close resources)
> >
> > I’m also still wondering if “sharing the client” is required - what are
> the affects of having multiple clients that connect to the same instance?
> These clients will only operate on different collections within that
> instance right? These would connect to the same endpoint, sure, but is that
> a problem other than inflating the connection pool slightly? It might make
> things more consistent with other artifact stores, with the sacrifice of
> some azure purity that not have real impact.
> >
> > If there is significant problem with not revising the SPI to better
> handle shutdown (or other aspects), an alternate approach would be to
> address the SPI first.
> >
> > I’m interested to know what others think.
> >
> > Thanks
> > Tyson
> >
> > > On Jun 21, 2018, at 3:50 AM, Chetan Mehrotra <
> chetan.mehrotra@gmail.com> wrote:
> > >
> > > ArtifactStore SPI exposes a shutdown method which is responsible for
> > > closing any resource owned by store implementation.
> > >
> > > Ccurrently for CouchDbRestStore it only shuts down ActorMaterializer
> which
> > > is created one per instance. It does not shutdown Http pool which is
> shared
> > > across 3 store instance. This is documented in PoolingRestClient.
> > >
> > > Now with CosmosDBArtifactStore we need to share a `DocumentClient`
> instance
> > > which owns the underlying Netty connection pool. As all the store
> instance
> > > talk to same db it makes sense to share the instance.
> > >
> > > However this sharing poses problem with shutdown. To  handle that
> CosmosDB
> > > PR introduces a `CountedReference` [1] which keeps an open/close count
> and
> > > only closes when all references are closed.
> > >
> > > Wanted to check with team if that would be fine approach to take?
> > >
> > > Currently its bit tricky to manage components lifecycle and often we
> need
> > > to rely on shutdown hooks to close the resources properly. May be we
> > > revisit SPI/component lifecycle handling later and then review this
> > > shutdown method handling
> > >
> > > Chetan Mehrotra
> > > [1] https://na01.safelinks.protection.outlook.com/?url=
> https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-openwhisk%2Fpull%2F3562%
> 2Ffiles%23diff-&data=02%7C01%7Ctnorris%40adobe.com%
> 7Cfbbbb0ade8ac4925e92b08d5d764dd9c%7Cfa7b1b5a7b34438794aed2c178de
> cee1%7C0%7C0%7C636651750626753531&sdata=BJVvsBCyWCnzspYnYDyXrBfiLNk97v
> BJyJuDQzslcag%3D&reserved=0
> > > 9d57de71410575fd70240ac974be407d
> >
>

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