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

Improved fetching CoreData entities by batch of records, not by one #115

Conversation

vandrusik
Copy link

Function update(with syncSpace: SyncSpace) is processing each asset/entry one by one, and this cause to many CoreData fetches in case of many changes in Contentful since previous app launch.
Suggested improvements are fetching with IN predicate, which leads to decreasing count of fetches and speeding up the update process.

@mariuskatcontentful
Copy link
Collaborator

@vandrusik thanks for proposing the improvement on the SDK but some tests were reported as failing. Unless those are passing i am afraid i can not merge it.

@vandrusik
Copy link
Author

@vandrusik thanks for proposing the improvement on the SDK but some tests were reported as failing. Unless those are passing i am afraid i can not merge it.

@mariuskatcontentful
Hey, I have looked at logs, this is a problem with one test, with name testCanContinueSyncWithExistingToken.
It has error on master branch as well, so it seems not my changes issue.

In the test, you are comparing that entries.count with 0, but actual value is 1.
Looks like somebody has updated some Contentful entry in space dqpnpm0n4e75, and now the test is failing.
You should change the test to compare with 1 (line 111 in ContentfulPersistenceTests.swift), or to use fresh syncTokenId (line 106).

@mariuskatcontentful
Copy link
Collaborator

Yes, sorry it was my bad i fixed the content in the cloud and now it should be fine. Are you able to add any tests to support your proposed changes for the future?

@vandrusik
Copy link
Author

vandrusik commented Jun 25, 2021

Yes, sorry it was my bad i fixed the content in the cloud and now it should be fine. Are you able to add any tests to support your proposed changes for the future?
@mariuskatcontentful
Hey, just added a test for new functionality, please take a look

@mariuskatcontentful
Copy link
Collaborator

@vandrusik incorporated your ideas in separate commit and made a new sdk release - hope that helps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants