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: KeyValueStore.recordExists() #2339

Merged
merged 3 commits into from
Feb 19, 2024
Merged

feat: KeyValueStore.recordExists() #2339

merged 3 commits into from
Feb 19, 2024

Conversation

barjin
Copy link
Contributor

@barjin barjin commented Feb 16, 2024

Testing for record existence with getValue() puts an unnecessary load on the Apify Platform / the storage backend - the client has to load the value just to throw it away in the next step.

recordExists() tests for the existence of a KVS key-value pair without actually retrieving the value itself.


side note: we should probably bump the apify-client dependency in apify-sdk, release a new version(?) and have Crawlee depend on this (and greater) versions of apify-sdk, right?

Also, it's been some time since I did something with the storages, what's the procedure there? Are we still maintaining the apify-storage-local package - if so, we should probably add this method there as well

@barjin barjin added the adhoc Ad-hoc unplanned task added during the sprint. label Feb 16, 2024
@barjin barjin requested review from B4nan and vladfrangu February 16, 2024 10:25
@barjin barjin self-assigned this Feb 16, 2024
@github-actions github-actions bot added this to the 83rd sprint - Tooling team milestone Feb 16, 2024
@github-actions github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Feb 16, 2024
@barjin
Copy link
Contributor Author

barjin commented Feb 16, 2024

Thanks for the approval, but also please see the description if you have the time 😄 What is our modus operandi when updating storage, and what changes need to be made and where? The more I think about it, the less I remember how we do this.

@vladfrangu
Copy link
Member

side note: we should probably bump the apify-client dependency in apify-sdk, release a new version(?) and have Crawlee depend on this (and greater) versions of apify-sdk, right?

Sounds fair to me, not sure what @B4nan thinks about it 👀


Also, it's been some time since I did something with the storages, what's the procedure there? Are we still maintaining the apify-storage-local package - if so, we should probably add this method there as well

We still are supposed to maintain it, yes (even tho I also don't like it 😢). If you've got more burning stuff on your plate I can handle that once we merge this in!

@barjin
Copy link
Contributor Author

barjin commented Feb 16, 2024

Cheers, that's what I thought 👍🏽 but we should probably have it in the local-storage first (before merging this), so we don't try to call a non-implemented storage method from Crawlee, no? I can make that PR now, just wanted to check if there isn't anything else to keep in mind

@B4nan
Copy link
Member

B4nan commented Feb 16, 2024

Yes and yes, we want to require the latest client in all of the places that require the new API, and this needs to work with the sqlite storage too.

@barjin barjin merged commit 8507a65 into master Feb 19, 2024
8 checks passed
@barjin barjin deleted the feat/record-exists branch February 19, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. 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.

3 participants