Skip to content

Auto retry read-only queries outside transactions#243

Merged
fmoor merged 5 commits intomasterfrom
retry-readonly
Nov 1, 2021
Merged

Auto retry read-only queries outside transactions#243
fmoor merged 5 commits intomasterfrom
retry-readonly

Conversation

@fmoor
Copy link
Member

@fmoor fmoor commented Sep 24, 2021

fixes #239

fmoor
fmoor commented Sep 24, 2021
View reviewed changes
edgedb/asyncio_con.py Outdated
# if the new impl fails to connect it will be in an inconsistent
# state. Use the old impl instead because it is in a consistent
# state.
self._inner._impl = impl
Copy link
Member Author

@fmoor fmoor Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the best way to solve this 😕

Copy link
Member

@elprans elprans Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call _AsyncIOConnectionImpl.terminate() and make sure the latter sets _protocol to None, which would make is_closed() return True and there should be no inconsistency downstream.

Copy link
Member Author

@fmoor fmoor Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

Copy link
Contributor

@tailhook tailhook Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the context of this? Github doesn't show previous commits any more.

The impl is designed as throw off thing (if connection is broken impl should be replaced with the new one). And is_closed() after connect fails must be invariant of the impl itself (if it's not, this is a bug, let's fix it).

And/or let's make a _AsyncIOConnectionImpl.connect a static function/constructor (because we can't do real constructors awaitable in python), so we don't even assign it to the connection if connection was not established.

Copy link
Contributor

@tailhook tailhook Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I see, we don't ensure that all gone well on timeout or cancellation. Anyway, assigning to self._inner._impl after connection succeed looks like more robust solution.

Copy link
Member Author

@fmoor fmoor Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assigning to self._inner._impl after connection succeed looks like more robust solution.

changed

@fmoor fmoor requested review from elprans and fantix September 24, 2021 17:53
@fmoor fmoor force-pushed the retry-readonly branch 5 times, most recently from 68437c3 to c89ca2f Compare October 4, 2021 16:38
@elprans elprans requested a review from 1st1 October 25, 2021 21:18
@1st1
Copy link
Member

1st1 commented Oct 28, 2021

@tailhook Paul, can you please take a look at this? I'll also do a review, but I'm less knowledgeable in this code now.

@1st1 1st1 requested a review from tailhook October 28, 2021 03:13
1st1
1st1 reviewed Oct 28, 2021
View reviewed changes
1st1
1st1 reviewed Oct 28, 2021
View reviewed changes
tailhook
tailhook reviewed Oct 28, 2021
View reviewed changes
Copy link
Contributor

@tailhook tailhook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the biggest question so far is why we have a separate capabilities cache? I think query cache should keep capabilities field around.

Do you foresee bigger capability cache than query cache?

@fmoor
Copy link
Member Author

fmoor commented Oct 28, 2021

I think query cache should keep capabilities field around.

I removed the capabilities cache and added capabilities to the query cache.

@fmoor fmoor requested a review from tailhook October 28, 2021 21:43
tailhook
tailhook approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@tailhook tailhook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix reconnect condition. Other than that, looks good to me.

@fmoor fmoor merged commit 76bb586 into master Nov 1, 2021
@fmoor fmoor deleted the retry-readonly branch November 1, 2021 19:11
@fmoor fmoor mentioned this pull request Nov 8, 2021
Merged
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
7f96277 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup edgedb.toml recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement EDGEDB_CLIENT_SECURITY
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

Deprecations
============

* Deprecate Pool.acquire() and Pool.release()
  (by @fmoor in 2d501e9 for #217)

* Rename tls_verify_hostname to tls_security
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in connection_lost if it has been already
  (by @elprans in c719e79 for #215)

* Retry if start() raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of SSLContext (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: _borrowed_for is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix retrying_transaction() on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on AsyncIOPool (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
2272824 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup `edgedb.toml` recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement `EDGEDB_CLIENT_SECURITY`
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

Deprecations
============

* Deprecate `Pool.acquire()` and `Pool.release()`
  (by @fmoor in 2d501e9 for #217)

* Rename `tls_verify_hostname` to `tls_security`
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in `connection_lost` if it has been already
  (by @elprans in c719e79 for #215)

* Retry if `start()` raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of `SSLContext` (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: `_borrowed_for` is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix `retrying_transaction()` on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on `AsyncIOPool` (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
@elprans elprans mentioned this pull request Nov 12, 2021
Merged
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
a854632 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup `edgedb.toml` recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement `EDGEDB_CLIENT_SECURITY`
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

Deprecations
============

* Deprecate `Pool.acquire()` and `Pool.release()`
  (by @fmoor in 2d501e9 for #217)

* Rename `tls_verify_hostname` to `tls_security`
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in `connection_lost` if it has been already
  (by @elprans in c719e79 for #215)

* Retry if `start()` raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of `SSLContext` (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: `_borrowed_for` is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix `retrying_transaction()` on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on `AsyncIOPool` (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
36efb5f 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup `edgedb.toml` recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement `EDGEDB_CLIENT_SECURITY`
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

Deprecations
============

* Deprecate `Pool.acquire()` and `Pool.release()`
  (by @fmoor in 2d501e9 for #217)

* Rename `tls_verify_hostname` to `tls_security`
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in `connection_lost` if it has been already
  (by @elprans in c719e79 for #215)

* Retry if `start()` raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of `SSLContext` (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: `_borrowed_for` is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix `retrying_transaction()` on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on `AsyncIOPool` (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 left review comments

@fantix fantix Awaiting requested review from fantix

@elprans elprans Awaiting requested review from elprans

+1 more reviewer

@tailhook tailhook tailhook approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Read only statements outside of transactions should be retried automatically

4 participants

@fmoor @1st1 @tailhook @elprans