You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openwhisk.apache.org by Chetan Mehrotra <ch...@gmail.com> on 2018/06/21 10:50:45 UTC
ArtifactStore shutdown handling and shared resources
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://github.com/apache/incubator-openwhisk/pull/3562/files#diff-
9d57de71410575fd70240ac974be407d
Re: ArtifactStore shutdown handling and shared resources
Posted by Tyson Norris <tn...@adobe.com.INVALID>.
Thanks for chiming in Rodric - can you add a review to the PR https://github.com/apache/incubator-openwhisk/pull/3562?
I mostly want to avoid adding the ref count complexity that will be removed, but if that isn’t a concern for anyone else I will get it merged, but would like the review for a sanity check.
Thanks
Tyson
On Jun 26, 2018, at 6:18 AM, Rodric Rabbah <ro...@gmail.com>> wrote:
+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 <ch...@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 <tn...@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<ma...@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<http://2Fgithub.com>%2Fapache%2Fincubator-openwhisk%2Fpull%2F3562%
2Ffiles%23diff-&data=02%7C01%7Ctnorris%40adobe.com<http://40adobe.com>%
7Cfbbbb0ade8ac4925e92b08d5d764dd9c%7Cfa7b1b5a7b34438794aed2c178de
cee1%7C0%7C0%7C636651750626753531&sdata=BJVvsBCyWCnzspYnYDyXrBfiLNk97v
BJyJuDQzslcag%3D&reserved=0
9d57de71410575fd70240ac974be407d
Re: ArtifactStore shutdown handling and shared resources
Posted by Rodric Rabbah <ro...@gmail.com>.
+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 <ch...@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 <tn...@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
> >
>
Re: ArtifactStore shutdown handling and shared resources
Posted by Chetan Mehrotra <ch...@gmail.com>.
> 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 <tn...@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 <ch...@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%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636651750626753531&sdata=BJVvsBCyWCnzspYnYDyXrBfiLNk97vBJyJuDQzslcag%3D&reserved=0
> > 9d57de71410575fd70240ac974be407d
>
Re: ArtifactStore shutdown handling and shared resources
Posted by Tyson Norris <tn...@adobe.com.INVALID>.
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 <ch...@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%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636651750626753531&sdata=BJVvsBCyWCnzspYnYDyXrBfiLNk97vBJyJuDQzslcag%3D&reserved=0
> 9d57de71410575fd70240ac974be407d