-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Replace aiohttp.ClientSession with AlloyDBAdminAsyncClient #416
base: main
Are you sure you want to change the base?
feat: Replace aiohttp.ClientSession with AlloyDBAdminAsyncClient #416
Conversation
noxfile.py
Outdated
@@ -126,7 +126,7 @@ def cover(session): | |||
def default(session, path): | |||
# Install all test dependencies, then install this package in-place. | |||
session.install("-r", "requirements-test.txt") | |||
session.install("-e", ".") | |||
session.install(".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting the following error if using session.install("-e", ".")
:
ImportError while loading conftest '/usr/local/google/home/rhatgadkar/alloydb-python-connector/tests/unit/conftest.py'.
tests/unit/conftest.py:22: in <module>
from mocks import FakeAlloyDBClient
tests/unit/mocks.py:33: in <module>
from google.cloud.alloydb.connector.connection_info import ConnectionInfo
E ModuleNotFoundError: No module named 'google.cloud.alloydb.connector'
Maybe there’s a conflict when importing the google-cloud-alloydb-connector
package when the google-cloud-alloydb
package is installed. But when changing to session.install(".")
, this error doesn’t occur anymore.
When trying to connect to an instance using this PR’s change, am getting the following error:
Followed the work-around in telepresenceio/telepresence#1871 to use "native" GRPC DNS resolver, but still getting similar error:
Need to figure out how to resolve this error. Maybe it's related to the options we're passing when constructing |
Issue is now fixed in latest commit. When passing in the API endpoint in the |
44d084b
to
197816e
Compare
mypy.ini
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding these because was getting an error when importing google.cloud.alloydb_v1beta
and google.api_core.*
libraries. Followed the approach here: https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-library-stubs-or-py-typed-marker
tests/unit/test_packaging.py
Outdated
google_cloud_alloydb = tmp_path / "google" / "cloud" / "alloydb" | ||
google_cloud_alloydb.mkdir() | ||
google_cloud_alloydb.joinpath("othermod.py").write_text("") | ||
env = dict(os.environ, PYTHONPATH=str(tmp_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this part of the test because it was failing. I think it fails because google.cloud.alloydb
is not a namespace package anymore. When the othermod.py
file is created in tmp_path
, I think it doesn't look inside tmp_path
when calling python -m google.cloud.alloydb.othermod
, and it instead looks inside the directory where google.cloud.alloydb
was installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've put this in a separate PR in #428.
I did a performance test by creating a script to create 500 connections: connect.py.zip I ran the script twice: when using the
|
Proper error message was returned when entering an invalid resource URI:
Similarly, an error is correctly returned when entering an invalid username:
|
bbcea6b
to
bb1285a
Compare
noxfile.py
Outdated
"xml", | ||
"-o", | ||
"sponge_log.xml", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this change because was getting the following error in Windows unit tests:
Run nox -s unit-3.13
nox > Running session unit-3.13
nox > Creating virtual environment (virtualenv) using python.exe in .nox\unit-3-13
nox > python -m pip install -r requirements-test.txt
nox > python -m pip install .
nox > python -m pip install -r requirements.txt
nox > pytest --cov=google.cloud.alloydb.connector -v --cov-config=.coveragerc --cov-report= --cov-fail-under=0 --junitxml=sponge_log.xml 'tests\unit'
ImportError while loading conftest 'D:\a\alloydb-python-connector\alloydb-python-connector\tests\unit\conftest.py'.
tests\unit\conftest.py:21: in <module>
from mocks import FakeAlloyDBClient
tests\unit\mocks.py:26: in <module>
from cryptography import x509
.nox\unit-3-13\Lib\site-packages\cryptography\x509\__init__.py:7: in <module>
from cryptography.x509 import certificate_transparency, verification
.nox\unit-3-13\Lib\site-packages\cryptography\x509\certificate_transparency.py:8: in <module>
from cryptography.hazmat.bindings._rust import x509 as rust_x509
E ImportError: PyO3 modules compiled for CPython 3.8 or older may only be initialized once per interpreter process
nox > Command pytest --cov=google.cloud.alloydb.connector -v --cov-config=.coveragerc --cov-report= --cov-fail-under=0 --junitxml=sponge_log.xml 'tests\unit' failed with exit code 4
nox > Session unit-3.13 failed.
Error: Process completed with exit code 1.
Followed the workaround in pytest-dev/pytest-cov#614 to solve this issue. It states that the module is being initialized twice because of the --cov
parameter in pytest
. So it suggests to use the coverage run
command instead with the --include
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave a comment in the code so future maintainers don't need to rediscover this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I've added the comment in #428
@@ -235,5 +235,3 @@ async def close(self) -> None: | |||
"""Helper function to cancel RefreshAheadCaches' tasks | |||
and close client.""" | |||
await asyncio.gather(*[cache.close() for cache in self._cache.values()]) | |||
if self._client: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer necessary? I would expect (at least from Go), that the client needs to be closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client doesn't have a close method: https://github.com/googleapis/google-cloud-python/blob/main/packages/google-cloud-alloydb/google/cloud/alloydb_v1beta/services/alloy_db_admin/async_client.py
noxfile.py
Outdated
"xml", | ||
"-o", | ||
"sponge_log.xml", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave a comment in the code so future maintainers don't need to rediscover this.
tests/unit/test_packaging.py
Outdated
google_cloud_alloydb = tmp_path / "google" / "cloud" / "alloydb" | ||
google_cloud_alloydb.mkdir() | ||
google_cloud_alloydb.joinpath("othermod.py").write_text("") | ||
env = dict(os.environ, PYTHONPATH=str(tmp_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a separate PR?
I would maybe also test 403 error for not having client permissions? That is an important one. |
403 error message is returned a proper error message too:
|
This is not an actionable error message, if this the actual error AlloyDB rest API is returning then you may want to file an internal bug to improve error message. Error message should point at missing permission or missing API enablement etc so customer can act on it. |
|
These tests were for #400. But those changes are being removed, so we can remove those tests.
530a338
to
a3800ba
Compare
This PR replaces calls to the AlloyDB admin API that use
aiohttp.ClientSession
to use thegoogle.cloud.alloydb
package'sAlloyDBAdminAsyncClient
instead.The dependencies for this change were added in #428.
Did a performance test of this change by creating a script to create 500 connections. I ran the script twice: when using the
AlloyDBAdminAsyncClient
and when usingaiohttp.ClientSession
. Got similar results:Because the performance is very similar, we can use the
AlloyDBAdminAsyncClient
.In the error case, for example, when passing an invalid instance URI, it was verified that the
AlloyDBAdminAsyncClient
returns a proper error message: