-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Source Monday: fix boards stream 25 row limit in source-monday #25277
Conversation
@@ -74,6 +74,7 @@ definitions: | |||
$parameters: | |||
name: "boards" | |||
path: "" | |||
items_per_page: 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current Pagination already uses 100 items check default_paginator
component. Are you able to build the dev version and test it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have run python main.py read --config secrets/config.json --catalog configured_stream.json
. It works for us with 200+ boards.
This is not only about pagination, but also about limit
parameter;
airbyte/airbyte-integrations/connectors/source-monday/source_monday/manifest.yaml
Line 22 in 44fa284
limit: "{{ parameters['items_per_page'] }}" |
Right now the logic is following:
limit
becomes None in here- Because
limit
is not specified, Monday returns default 25 rows forboards
. - Pagination checks if it needs to visit another page with the condition:
len(last_records) < self.page_size
. And it gets25 < 100
. So there is no query for the second page.
This PR solves the issue: the limit is set and pagination starts working when needed. Although it does not look self-explaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(venv) igsaf@igsaf-nix:~/IdeaProjects/airbyte/airbyte-integrations/connectors/source-monday (fix-monday-boards) $ python main.py read --config secrets/config.json --catalog configured_stream.json | tail -n 4
{"type": "LOG", "log": {"level": "INFO", "message": "Read 221 records from boards stream"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Finished syncing boards"}}
{"type": "LOG", "log": {"level": "INFO", "message": "SourceMonday runtimes:\nSyncing stream boards 0:00:21.135193"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Finished syncing SourceMonday"}}
/test connector=connectors/source-monday
Build PassedTest summary info:
|
ping |
Hello @marcosmarxm, what are the roadblockers right now ? Do you have an ETA for the merge ? |
/publish connector=connectors/source-monday
if you have connectors that successfully published but failed definition generation, follow step 4 here |
@igsaf2 there is a bug in our CI. Team is fixing it and after I'll merge your contirbution. You can already update the connector version in the Airbyte UI. |
@marcosmarxm I resynced my connector but still had the problem of limits (I am limited to 100 and not 25...). Should I re-create my source or is there anything to do ? I might not be on the new connector. |
@mathieulebasic It seems to be a question about how to update a connector, check out Airbyte slack workspace, it is great. |
…tehq#25277) * fix boards stream 25 row limit in source-monday * update docs and dockerfile * auto-bump connector version --------- Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
What
Resolves #24194
How
The problem in the default 25 item limit. E.g. you can see it in monday forum.
The Monday connector uses
limit
parameter to configure the number of entries to fetch (function)This limit is set to be equal
items_per_page
parameter. (manifest link).items_per_page
must be not less thanpage_size
in paginator (link), because otherwisenext_page_token
method will returnNone
(pagination strategy)🚨 User Impact 🚨
I believe there are no breaking changes
Pre-merge Checklist
Expand the relevant checklist and delete the others.
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
Connector version has been incremented
Dockerfile
has updated versionDocumentation updated
README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
with an entry for the new version. See changelog examplePR name follows PR naming conventions
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described here