CDK: private configuration option _limit and _page_size #5617

avida · 2021-08-25T11:07:17Z

What

Resolves #4985 issue

How

Describe the solution

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

Zirochkaa

lgtm, just small comment

Zirochkaa · 2021-08-26T17:23:37Z

airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py

@@ -135,6 +147,9 @@ def _read_stream(
        for record in record_iterator:
            if record.type == MessageType.RECORD:
                record_counter += 1
+                if internal_config.limit and record_counter > internal_config.limit:
+                    logger.info("Reached limit defined by internal config, stop reading")


Could you please add number, like so:

Suggested change

logger.info("Reached limit defined by internal config, stop reading")

logger.info(f"Reached limit defined by internal config ({internal_config.limit}), stop reading")

Phlair

Like this idea. The limit keyword makes total sense and could be used by any cdk stream off the bat with this change, nice!

For page_size, iiuc, it needs to be individually implemented per stream for this setting to have any effect? I think right now it's not entirely clear what page_size actually is... is it a limit to number of pages to paginate through? Is it limit for number of records per page? Could either / both of those settings be useful depending on the connector?
Ultimately, I'm wondering if just implementing the ability to create any _internal_setting would provide more flexibility, including allowing implementing a _page_size if needs be, but since there's no high-level logic changes for it, it could fit into the generic box too.

Phlair · 2021-08-27T13:46:13Z

airbyte-cdk/python/airbyte_cdk/sources/utils/schema_helpers.py

+
+
+class InternalConfig(BaseModel):
+    KEYWORDS: ClassVar[set] = {"_limit", "_page_size"}


I can see why we'd be explicit here to avoid accidentally putting actual config values in the internal config... however, in adding this feature does it perhaps make sense to enable adding any internal flags without the need to update cdk code each time (e.g. taking any keywords that start with _)?
This is more of an open question than a suggestion, so wdyt?

See my overall review comment which links to this :)

For page_size, iiuc, it needs to be individually implemented per stream for this setting to have any effect? I think right now it's not entirely clear what page_size actually is... is it a limit to number of pages to paginate through? Is it limit for number of records per page? Could either / both of those settings be useful depending on the connector?

Its number of record per page. Right now its not a standard property but some streams use page_size name to configure number of records per page. It works only for instances of HttpStream class.

Ultimately, I'm wondering if just implementing the ability to create any _internal_setting would provide more flexibility, including allowing implementing a _page_size if needs be, but since there's no high-level logic changes for it, it could fit into the generic box too.

Do you mean filtering out records that starts with _ ? In this case we need somehow distinguish internal config for cdk and connector itself. We don't want to filter out connector config and also we don't want to pass cdk config to the connector.

I can see why we'd be explicit here to avoid accidentally putting actual config values in the internal config... however, in adding this feature does it perhaps make sense to enable adding any internal flags without the need to update cdk code each time (e.g. taking any keywords that start with _)?
This is more of an open question than a suggestion, so wdyt?

Its good suggestion but there is some connectors that using its own internal config. From the connector side we can set additionalproperties flag to true for spec jsonschema and we are sure that any config with any internal properties will pass checking against spec schema. But for cdk we need to filter out cdk specific configs to make this check pass, that's why there is split_config function. It still allows adding connector specific internal parameters but the should not be same as ones that as used for cdk. If we going to make it too generic it would be a mess with cdk and connector paramters

Phlair

Thanks for detailed responses on previous review. Understand and happy with approach based on that.

One thing I think is needed for clarity in adding these is detail around the availability of these flags, descriptions and how to use (for example, in the CDK tutorial in the docs maybe add a section on it?)

Phlair

Meant to approve with comment above ^

…-SAT-keys

avida · 2021-08-31T09:09:19Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/1185502392
✅ https://github.com/airbytehq/airbyte/actions/runs/1185502392

avida requested review from htrueman, Zirochkaa and yevhenii-ldv August 25, 2021 11:07

github-actions bot added the CDK Connector Development Kit label Aug 25, 2021

avida force-pushed the drezchykov/4985-add-SAT-keys branch 3 times, most recently from a780d0f to bdebac1 Compare August 25, 2021 11:37

Zirochkaa approved these changes Aug 26, 2021

View reviewed changes

CDK: private configuration option _limit and _page_size

e36793f

avida force-pushed the drezchykov/4985-add-SAT-keys branch from bdebac1 to e36793f Compare August 27, 2021 12:41

avida requested a review from sherifnada August 27, 2021 12:41

sherifnada requested review from Phlair and removed request for sherifnada August 27, 2021 13:39

Phlair reviewed Aug 27, 2021

View reviewed changes

avida requested a review from Phlair August 27, 2021 16:01

Phlair reviewed Aug 30, 2021

View reviewed changes

Phlair approved these changes Aug 30, 2021

View reviewed changes

Dmytro Rezchykov added 2 commits August 31, 2021 12:06

fix reviewer comments

e691c14

Merge remote-tracking branch 'origin/master' into drezchykov/4985-add…

e393551

…-SAT-keys

avida merged commit 7584440 into master Aug 31, 2021

avida deleted the drezchykov/4985-add-SAT-keys branch August 31, 2021 09:16

jrhizor mentioned this pull request Sep 3, 2021

Bump Airbyte version from 0.29.13-alpha to 0.29.14-alpha #5831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDK: private configuration option _limit and _page_size #5617

CDK: private configuration option _limit and _page_size #5617

avida commented Aug 25, 2021

Zirochkaa left a comment

Zirochkaa Aug 26, 2021

avida Aug 27, 2021

Phlair left a comment

Phlair Aug 27, 2021

Phlair Aug 27, 2021

avida Aug 27, 2021

Phlair left a comment

Phlair left a comment

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading

	logger.info("Reached limit defined by internal config, stop reading")
	logger.info(f"Reached limit defined by internal config ({internal_config.limit}), stop reading")



		class InternalConfig(BaseModel):
		KEYWORDS: ClassVar[set] = {"_limit", "_page_size"}

CDK: private configuration option _limit and _page_size #5617

CDK: private configuration option _limit and _page_size #5617

Conversation

avida commented Aug 25, 2021

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Zirochkaa left a comment

Choose a reason for hiding this comment

Zirochkaa Aug 26, 2021

Choose a reason for hiding this comment

avida Aug 27, 2021

Choose a reason for hiding this comment

Phlair left a comment

Choose a reason for hiding this comment

Phlair Aug 27, 2021

Choose a reason for hiding this comment

Phlair Aug 27, 2021

Choose a reason for hiding this comment

avida Aug 27, 2021

Choose a reason for hiding this comment

Phlair left a comment

Choose a reason for hiding this comment

Phlair left a comment

Choose a reason for hiding this comment

avida commented Aug 31, 2021 • edited by github-actions bot Loading

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading