+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.mehro...@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 <tnor...@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.mehro...@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 > > >
