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

feat: SQLTarget connector instance shared with sinks #1864

Conversation

BuzzCutNorman
Copy link
Contributor

@BuzzCutNorman BuzzCutNorman commented Jul 20, 2023

This adds a variable _target_connector to SQLTarget to house an instance of the default_sink_class.connector_class and a property to access it. The method get_sink was brought down from Target and altered to call add_sqlsink. The method add_sqlsink is a copy of the add_sink method in Target it just passes the shared connector instance to the SQLSink isntances it initalizes for each stream.

need some assistance with making this mypy compliant.

Closes #1772


📚 Documentation preview 📚: https://meltano-sdk--1864.org.readthedocs.build/en/1864/

@BuzzCutNorman BuzzCutNorman changed the title 1772-sqltarget-level-connector-instance-shared-with-sinks feat: SQLarget connector instance shared with sinks Jul 20, 2023
@BuzzCutNorman BuzzCutNorman changed the title feat: SQLarget connector instance shared with sinks feat: SQLTarget connector instance shared with sinks Jul 22, 2023
@edgarrmondragon
Copy link
Collaborator

@BuzzCutNorman can you try fixing the mypy failure? Otherwise I can take look in the coming days.

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #1864 (e628032) into main (b1b3bd2) will increase coverage by 0.02%.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
+ Coverage   86.87%   86.89%   +0.02%     
==========================================
  Files          59       59              
  Lines        5027     5060      +33     
  Branches      814      819       +5     
==========================================
+ Hits         4367     4397      +30     
- Misses        464      466       +2     
- Partials      196      197       +1     
Files Changed Coverage Δ
singer_sdk/target_base.py 91.93% <91.17%> (-0.16%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BuzzCutNorman
Copy link
Contributor Author

@edgarrmondragon I was able to get the mypy failures resolved.

Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

Thank you @BuzzCutNorman, overall this is looking great. I still have to go through the API and see how this fits with our plans for connectors (at first glance it seems it does!).

tests/core/test_target_base.py Outdated Show resolved Hide resolved
tests/core/test_target_base.py Outdated Show resolved Hide resolved
singer_sdk/target_base.py Show resolved Hide resolved
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

It doesn't look like we need to update the cookiecutter templates, can you confirm that?

@BuzzCutNorman
Copy link
Contributor Author

BuzzCutNorman commented Jul 27, 2023

You are correct the cookiecutter templates do not need updating. In my initial version they did need to be updated. Then thanks to the sdk test I realized there was a way to accomplish this without adding a default_connector_class variable. This also means target developers won't need to make any code changes to their targets for the target to benefit from this PR.

@edgarrmondragon
Copy link
Collaborator

You are correct the cookiecutter templates do not need updating. In my initial version they did need to be updated. Then thanks to the sdk test I realized there was a way to accomplish this without adding a default_connector_class variable. This also means target developers won't need to make any code changes to their targets for the target to benefit from this PR.

Thanks for confirming!

@edgarrmondragon
Copy link
Collaborator

Thanks @BuzzCutNorman!

@edgarrmondragon edgarrmondragon merged commit 759c77b into meltano:main Jul 28, 2023
25 checks passed
@BuzzCutNorman BuzzCutNorman deleted the 1772-sqltarget-level-connector-instance-shared-with-sinks branch November 28, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Target level SQLConnector instance shared with Sinks
3 participants