Skip to content

refactor: Introduce new Apify storage client #470

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

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

vdusek
Copy link
Contributor

@vdusek vdusek commented May 10, 2025

Description

Issues

Testing

  • The current test set covers the changes.

@vdusek vdusek self-assigned this May 10, 2025
@github-actions github-actions bot added this to the 114th sprint - Tooling team milestone May 10, 2025
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label May 10, 2025
@vdusek vdusek changed the title New apify storage clients refactor: Introduce new Apify storage client May 10, 2025
@vdusek vdusek force-pushed the new-apify-storage-clients branch from d27c080 to 82efd3e Compare June 12, 2025 12:44
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Jun 18, 2025
@vdusek vdusek force-pushed the new-apify-storage-clients branch 2 times, most recently from 067b793 to 104a168 Compare June 23, 2025 09:12
@vdusek vdusek marked this pull request as ready for review June 26, 2025 13:04
@vdusek vdusek requested a review from Pijukatel June 26, 2025 13:05
@janbuchar janbuchar self-requested a review June 26, 2025 13:27
@@ -11,14 +11,14 @@ async def main() -> None:
await dataset.export_to(
content_type='csv',
key='data.csv',
to_key_value_store_name='my-cool-key-value-store',
to_kvs_name='my-cool-key-value-store',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this BC break worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's evaluate all the potential BCs at the end

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I thought we are nearing that now 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're just re-exporting the storages from crawlee here, there will be many more cases than this one. I'm not saying we have to rename this particular argument (and I will undo it if you insist—just I don't like those long identifiers, especially when we can use the KVS abbreviation).

@Pijukatel Pijukatel self-requested a review July 17, 2025 13:23
Copy link
Contributor

@Pijukatel Pijukatel left a comment

Choose a reason for hiding this comment

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

While testing the change on platform I used modified benchmark tests. They are specific, because they scrape locally hosted site and so the speed of scraping is unusually fast - no network related delays. This exposes RequestQueue race condition that can happen when checking if the RQ is empty or not.
When running the test using this branch, I saw that the crawler premature finished even though RQ was still quite full. The hypothesis about premature exit was confirmed when running same test on branch modified with extra log and sleep.

Please see following tiny commit based on this branch and attached log:
5b25728

image

From this run: https://console.apify.com/actors/UdJ0NQp2j8140G9db/runs/4yfZqLvLo1xhrk2Cb#log

@vdusek vdusek force-pushed the new-apify-storage-clients branch from 2f39b35 to ca72313 Compare July 20, 2025 18:49
@Pijukatel Pijukatel self-requested a review July 22, 2025 13:45
@Pijukatel Pijukatel self-requested a review July 24, 2025 14:43
vdusek added a commit that referenced this pull request Jul 28, 2025
- Add more integration tests for RQ (mostly to better test the new Apify
RQ in the #470).
- Utilization of `Actor.log` - as they are seen in the test output.
@vdusek vdusek force-pushed the new-apify-storage-clients branch from d59b100 to f5189c5 Compare August 5, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce new Apify storage client
4 participants