Hi Julian, When designing protocol adapters they shall be designed with thread safety in mind. So if we get issues here we should take care of if. Nevertheless regarding you comment (and you are quite right with that): Keep using a returned (or closed) connection from a pool is not what the designer had in mind. Therefore I implemented connection invalidation with „08dd73e <https://github.com/apache/incubator-plc4x/commit/08dd73ee27e785537fe1710fed8314b18c05f0f0>“ and added a test-case. This should prevent reuse of the connection. As far as I know using the proxy this way (using anonym inner class with final parameters opposed to using a dedicated class) one can’t access the contained connection via reflection and therefore can’t circumvent the invalidation.
Regarding that request limiting is see it similar to you (above, or below [shall be default message based and per protocol on other metrics if possible])… Regarding the ProxyConnection I state above why I prefer the proxy there (could be non proxy implementation too). Yep regarding the hook no objections against that (What we could discuss though if the isConnected() should include that or if the semantic of this is a layer below). Sebastian > Am 26.10.2018 um 15:54 schrieb Julian Feinauer <j.feina...@pragmaticminds.de>: > > Hey Sebastian, > > > the only problem witht he reuse i had is possible internal state during > in-flight messages or problems with multi threading (I'm not sure if all > drivers are thread safe and stateless during in-flight messages), but I agree > that this is minor currently, just wanted to add the todo to keep it in view > : ) > > Request Limiting is, as I think, no concern of the driver itself, I think, so > I would add request limiting probably in an layer above thus I'm totally fine. > > > With regards to Marcels branch I thought it would be ideal to move over its > ProxyConnection as this is everything thats needed to fix the todo (all > methods delegate untill close() is called then all throw an unsupported > operation exception. This is similar to what common jdbc pools do, I checked > dbcp and some other one). > > > Yes, we can directly add the validation hook from commons pool, but I think > we need a plc specific ping method in the api to call as we have no "common" > field or "query" we can issue. > > > Best > > Julian > > ________________________________ > Von: Sebastian Rühl <sebastian.ruehl...@googlemail.com.INVALID> > Gesendet: Freitag, 26. Oktober 2018 13:23:55 > An: dev@plc4x.apache.org > Betreff: Re: OPM / Connection Pool > > Hi Julian, > > Regarding the reuse of Connections: > I was aware of that issue but forgot to add a TODO for that (sorry for that). > But in general a pool is not a request limiter rater a connection limiter. > Therefore the continue use shouldn’t be that big of an issue for the time > being (Till we resolve that issue). > > Request Limiting: > A separate generic request limiter feature would be nice but is out-of-scope > of the pool. In case of S7 this is builtin (maxAmqCaller/maxAmqCallee). > > Regarding the branch of Marcel (And thanks for the great contribution Marcel > :): > He did some good work but at the end the implementation was initially too > different from the commons-pool one as I would have reverted/changed almost > every line. So the idea was it to write it upfront in the separate module to > have the possibility to compare it side by side and pick the goodies. > > Regarding validation: > The commons-pool validation calls isConnected() on return. So we could > implement the „ping“ in the isConnected() as well or implement a ping() as > you described. > > Sebastian > >> Am 25.10.2018 um 23:13 schrieb Julian Feinauer >> <j.feina...@pragmaticminds.de>: >> >> Hey all, >> >> just wanted to keep you up to date with some things Sebastian and I >> discussed off-list. >> Thanks to Sebastians support the Object Plc Mapping (opm) is now mergend >> into master and I would love to get some feedback. >> I hope that I can soon add some documentation on the side about that and I >> would be happy if someone likes to implement writing (currently only reading >> is supported), see PLC4X-70. >> >> Sebastian also added an implementation for the Connection Pool. This can >> become one of the most important features to make it usable in enterprise >> applications (pooling, keep connections open, …). >> It is important to note that Marcel opened a PR for the same feature and my >> impression is that both implementations are lacking some (different) things >> and thus, together, are a pretty good first shot. >> Thus, I suggest that we keep PR-30 open [1] or at least the branch intact >> and move the Proxy Implementations over (added a TODO in the >> PooledPlcDriverManager which is dangerous, I think) and also add some >> validation (this should be added to the driver interface or the SPI >> somewhere, I guess to be able to have a reasonable “ping” for all plc’s, it >> is in our case not as easy as a “SELECT 1” in JDBC). >> >> Thus, we are on a great way, and as soon as we have moved over the necessary >> parts from PR-30 and probably also added the validation I would be very >> happy to ship OPM as feature in one of our applications. I’ll then also >> prepare a sample on that because it makes PLC Connections really as easy as >> querying a Database via JPA. >> >> Thanks especially to Marcel and Sebastian! >> Julian >> >> [1] https://github.com/apache/incubator-plc4x/pull/30 >
