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

fix: Close SQL Alchemy connections. #453

Merged
merged 1 commit into from Mar 3, 2021

Conversation

davcamer
Copy link
Contributor

@davcamer davcamer commented Mar 3, 2021

Summary of Changes

This addresses #868, where extractions using the Snowflake extractor would
hand until the connection was closed. I've added close() calls to the other
SQL Alchemy based extractors since it is a general expectation of SQL Alchemy
and most of the infrastructure existed to do it already.

Tests

Is there a good way of testing these extractor classes without requiring the appropriate DB to be available?

Test coverage in this project is already significantly below the goal of 70%, so make test is failing for me. This happens regardless of these changes.

Documentation

Closing connections is an expected behavior, so I did not add specific documentation.

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test

This addresses #868, where extractions using the Snowflake extractor would
hand until the connection was closed. I've added close() calls to the other
SQL Alchemy based extractors since it is a general expectation of SQL Alchemy
and most of the infrastructure existed to do it already.

Signed-off-by: Dave Cameron <dcameron@digitalocean.com>
@feng-tao
Copy link
Member

feng-tao commented Mar 3, 2021

thanks for the fix, lgtm

@feng-tao feng-tao merged commit 25124c1 into amundsen-io:master Mar 3, 2021
@davcamer
Copy link
Contributor Author

davcamer commented Mar 3, 2021

There is now a 9 line run of code repeated across the 6 Extractors that wrap a SQLAlchemyExtractor with extra DB specific functionality. It's this bit at the end of init that sets up the SQLAlchemyExtractor, and now the close method as well.

        self._alchemy_extractor = SQLAlchemyExtractor()
        sql_alch_conf = Scoped.get_scoped_conf(conf, self._alchemy_extractor.get_scope()) \
            .with_fallback(ConfigFactory.from_dict({SQLAlchemyExtractor.EXTRACT_SQL: self.sql_stmt}))

        self._alchemy_extractor.init(sql_alch_conf)
        self._extract_iter: Union[None, Iterator] = None

    def close(self) -> None:
        self._alchemy_extractor.close()

This could be DRYed up with a superclass, something like SQLAlchemyWrappingExtractor maybe? But the section about config could also be a function in the sql_alchemy_extractor module, which would avoid the inheritance hierarchy. I'll add a commit to do that in a bit. I'll open a separate refactor PR to explore that.

@davcamer davcamer deleted the sql_alchemy_close branch March 3, 2021 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants