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

Evaluate performance impact of deprecated decorator in hot codepath #116

Open
aaronsteers opened this issue Dec 4, 2024 · 3 comments
Open

Comments

@aaronsteers
Copy link
Contributor

aaronsteers commented Dec 4, 2024

There is one deprecated decorator in the hot code path which was initially moved to the class level to reduce performance impact, and which I've moved back to the method level but commented-out to avoid any performance impact. Since migrating from the Deprecated third party library to the PEP 702 native support for @deprecated decorators, we might be in a better situation now with performance, but I didn't want to take a risk. I also didn't want to trigger a deprecated warning on the whole class if only one method is deprecated.

# Commented-out to avoid any runtime penalty, since this is used in a hot per-record codepath.
# To be evaluated for re-introduction here: https://github.com/airbytehq/airbyte-python-cdk/issues/116
# @deprecated(
# "Deprecated method `get_updated_state` as of CDK version 0.1.49. "
# "Please use explicit state property instead, see `IncrementalMixin` docs."
# )
def get_updated_state(
self, current_stream_state: MutableMapping[str, Any], latest_record: Mapping[str, Any]
) -> MutableMapping[str, Any]:
"""DEPRECATED. Please use explicit state property instead, see `IncrementalMixin` docs.

@aaronsteers
Copy link
Contributor Author

cc @artem1205 (fyi)

I wanted to move the deprecated warning back the method level but I didn't want to risk a performance hit. Compromise was to move it back, comment it out, add a notice to the method's docstring, and open this issue for follow up. Probably not a high pri, but wanted to share with you for visibility.

@artem1205
Copy link
Contributor

I'm okay with it.
Do we have any plans (deadlines) to remove deprecated methods from the codebase as we move forward with the One CDK concept?

@natikgadzhi
Copy link
Contributor

@artem1205 no deadlines, but the cleanup is VERY encouraged. I'm very curious about the list of potential performance improvements that you wanted to do, but did not have time to do, as well.

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

No branches or pull requests

3 participants