search

long2ice / asynch

An asyncio ClickHouse Python Driver with native (TCP) interface support.
https://github.com/long2ice/asynch
Apache License 2.0
186 stars 43 forks source link

Reconsider the `Connection`, `Cursor` and `Pool` classes in terms of unification and marking some public API deprecated. #111

Closed stankudrow closed 2 months ago

stankudrow commented 3 months ago

Due to the recent changes in the API of Pool class and fixes or refinements made in the Connection and [Dict]Cursor classes, the PR is meant to be the last PR before releasing asynch v0.2.5.

The new Pool API, after fixing the dangling connections in the PR #109, is meant to be less error-prone and simple enough (it is similar to the psycopg3 pool design and was a source of inspiration , my thanks to @dvarrazzo (sorry for tagging)). The older API methods must be deprecated and must be removed as soon as possible until the common API will be refined and adjusted to the DB API 2.0 standards (yet another reconsideration to be continued).

@long2ice , @gnomeby , @DaniilAnichin , @pufit , @KuzenkovAG, @DFilyushin, @Kludex and other contributors of asynch, I urge you to coathor this PR and judge it severely.

stankudrow commented 3 months ago

@long2ice , @KuzenkovAG, @DFilyushin, @gnomeby , @DaniilAnichin , @pufit, we can work this PR out after dealing with the PR #112 and rebasing. Let's discuss the idea of reconsidering the API: what must be deprecated and removed later, what is worthy of saving.

If this PR is merged, then we must stabilise the project and release the asynch v0.2.5 and, slowly but surely, refining the API in future releases.

stankudrow commented 2 months ago

@long2ice , this PR is ready for review, @pufit, @lxneng , @gnomeby , @DaniilAnichin , could you review these changes and judge them severely? The Exception hierarchy and codification to be refined in later releases.

Please pay attention to the following major details:

  1. ConnectionError in the asynch/connection.py module -> is it OK or to be replaced with DB-API v2.0 compliant exceptions?
  2. AsyncPoolError in the async/pool.py module -> the same question -> please consider each place where this exception is raised and tell if any other DB-API v2.0 compliant exception suits best to be thrown. Plus, what superclass to inherit better?

@dvarrazzo , could you share your thoughts as well? I am confused about the use cases for DB-API v2.0 exceptions. Only Warning, Error and NotSupportedError exceptions are crystal clear for me (for now). For example, concerning AsynchPoolError, which class to inherit -> InterfaceError (the asynch is a sort of interface) or OperationalError? Or maybe this class should not exist and in some pool use cases it is better to throw InterfaceError, or OperationalError or InternalError and so on.

Folks, help me, please.

stankudrow commented 2 months ago

@long2ice , maybe wait for v0.2.5 and then merge this PR, or, if it is big enough, break it into smaller PR? What do you think?

long2ice commented 2 months ago

Thanks! Great job!