Skip to content
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

AsyncDefog #61

Merged
merged 17 commits into from
Sep 12, 2024
Merged

AsyncDefog #61

merged 17 commits into from
Sep 12, 2024

Conversation

Muhammad18557
Copy link
Contributor

@Muhammad18557 Muhammad18557 commented Sep 10, 2024

For postgres and redshift, we have the asynchronous asyncpg to replace psycopg2
For mysql, we have aiomysql to replace mysql.converter
For snowflake, we use the same connector as it also offers a cur.execute_async method- we then poll the status of the query id to see if its done running otherwise just simulate an async sleep using await asyncio.sleep(1) .
For sqlserver, we use aioodbc which has the exact same signature as pyodbc but is asynchronous and must be awaited.

However, for Databricks and BigQuery, no native asynchronous alternatives were found. To mimic asynchronous behavior for these databases, we are using asyncio.to_thread to handle blocking operations in a non-blocking manner.

Methods of AsyncDefog have the exact same signature (name, params) but have to be awaited. The initial methods like save_connection_json, check_db_creds, to_base64_creds, from_base64_creds are kept as it is (not made async) but do let me know if those also need to be async and that will be a 3 mins fix (because it will just need a wrapper off its parent class's methods into an async function i.e async def function(): return self.function()).

create_table_ddl and create_ddl_from_metadata also made async under async_admin_methods for the sake of consistency even though they are not I/O bound or db intensive.

Copy link
Member

@rishsriv rishsriv left a comment

Choose a reason for hiding this comment

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

Thanks Abdullah! This looks very, very good. And +1 for the tests.

Could you please add native asynchronous mode for the following:

  • Snowflake: example here
  • SQLServer, by using aioodbc. Example here

After that, I can test and merge!

@Muhammad18557
Copy link
Contributor Author

Muhammad18557 commented Sep 10, 2024 via email

Copy link
Member

@rishsriv rishsriv left a comment

Choose a reason for hiding this comment

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

Finished testing and this looks great Abdullah! Thanks for adding this much needed feature!

@rishsriv rishsriv merged commit afb83c4 into main Sep 12, 2024
2 checks passed
@rishsriv rishsriv deleted the abdullah/async-defog branch September 12, 2024 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants