search

nikita-volkov / hasql-pool

A pool of connections for Hasql
http://hackage.haskell.org/package/hasql-pool
MIT License
17 stars 15 forks source link

New observation for connection at creation time #43

Closed steve-chavez closed 6 months ago

steve-chavez commented 6 months ago

Problem

There are some problems with using ReadyForUseConnectionStatus for when a pool connection is created and for when is returned to the pool. https://github.com/nikita-volkov/hasql-pool/blob/8a9389f22b08f7eff7443c2744cc1ff2320aa770/src/library/exposed/Hasql/Pool.hs#L167-L173 https://github.com/nikita-volkov/hasql-pool/blob/8a9389f22b08f7eff7443c2744cc1ff2320aa770/src/library/exposed/Hasql/Pool.hs#L207-L212

Proposal

How about a new observation when the pool connection is created, named as CreatedAndReadyForUseConnectionStatus?

nikita-volkov commented 6 months ago

Agreed. You've quickly found a spot in the design which I was not sure about.

The current design is based on a status model. The observation informs the user about the new status that a connection has entered. Such approach is not the lowest possible level of abstraction and does involve some interpretation on the library end. The use of ReadyForUseConnectionStatus for different state transitions is exactly a case of such interpretation. What I was trying to achieve with that approach was to provide for a more stable API. Evidently recognising the validity of your request here I've failed :)

A lower level of abstraction would be to emit events describing what happened without any interpretation on how it affected the status a connection. There would be a separate constructor for every observed event. In such case instead of a status model we would have had an event model. An we actually did have that at some point during the development of this feature, see https://github.com/nikita-volkov/hasql-pool/commit/fc48a102585a832f3021360c5588ef9854573cc3.

The way that I see it we now have two options on what to do next:

  1. Extend the status model with the status that you recommend.

  2. Consider the status model a failed experiment and switch to event-model.

Unfortunately both options would require a major version bump according to PVP. I'm inclining towards the second option, but please do provide your take on the matter. No need to restore the old code, just change the naming and claims of some current types.

nikita-volkov commented 6 months ago

I've just had an idea about a backward-compatible option. We can introduce a separate module with events and a separate configuration option for them. The current status mechanism can stay with the same API, but under the hood it will be reimplemented as an interpretation layer above the event-model.

The price for going this route will be more technical debt to maintain.

steve-chavez commented 6 months ago

Unfortunately both options would require a major version bump according to PVP. I'm inclining towards the second option, but please do provide your take on the matter. The price for going this route will be more technical debt to maintain.

From my side I would be fine with a breaking change, I still haven't released https://github.com/PostgREST/postgrest/pull/3420. Wouldn't want to cause maintenance burden.

nikita-volkov commented 6 months ago

I've decided to keep the status model for now to avoid jumping between designs and extended the ReadyForUse status. It's now released as 1.1.

Pleas do post back on whether this suffices. If not we can quickly do another release addressing that.

steve-chavez commented 6 months ago

Those seem enough to solve this issue. Many thanks!

Will close this for now and report back if there are any issues.