Conversation
Changes in CategoryBatchProcessor * Rename to CategoryBatchValidator * Handle errors inside * Return a ImmutablePair of valid drafts and valid keys from the validation method * Pass the batch of categoryDrafts in the validation method * Change constructor to init syncOptions and statistics fields
…y-ref' into #235_improve-caching-for-category-ref
…y-ref' into #235_improve-caching-for-category-ref # Conflicts: # src/main/java/com/commercetools/sync/categories/CategorySync.java # src/main/java/com/commercetools/sync/categories/helpers/CategoryBatchValidator.java
…y-ref' into #235_improve-caching-for-category-ref # Conflicts: # src/main/java/com/commercetools/sync/services/impl/CategoryServiceImpl.java # src/test/java/com/commercetools/sync/categories/CategorySyncTest.java
…y-ref' into #235_improve-caching-for-category-ref
…or all batch helpers, and create the concept for the caching improvement for the cacheKeyToId usages.
ahmetoz
changed the title
ahmetoz
changed the title
ahmetoz
changed the base branch from
#235_improve-caching-for-category-ref
to
master
4 tasks
ahmetoz
changed the title
JudeNiroshan
suggested changes
Sep 24, 2020
ahmetoz
requested review from
JudeNiroshan
and removed request for
butenkor and
salander85
JudeNiroshan
approved these changes
Sep 25, 2020
ahalberkamp
approved these changes
Sep 25, 2020
| @Nonnull final ReferencedKeys referencedKeys, | ||
| @Nonnull final CategoryDraft categoryDraft) { | ||
|
|
||
| referencedKeys.categoryKeys.add(categoryDraft.getKey()); |
Contributor
There was a problem hiding this comment.
it s still not removed
| return syncBatch(validDrafts, keyToIdCache); | ||
| } | ||
|
|
||
| final Map<String, String> productKeyToIdCache = cachingResponse.getKey(); |
Contributor
There was a problem hiding this comment.
Suggested change
| final Map<String, String> productKeyToIdCache = cachingResponse.getKey(); | |
| final Map<String, String> productKeyToIdCaches = cachingResponse.getKey(); |
| } | ||
|
|
||
| @Nonnull | ||
| private static Set<String> getReferencedKeysWithReferenceTypeId( |
Contributor
There was a problem hiding this comment.
Suggested change
| private static Set<String> getReferencedKeysWithReferenceTypeId( | |
| private static Set<String> getReferenceIdByReferenceTypeId( |
| * injected {@link io.sphere.sdk.client.SphereClient} and stores a mapping for every category to id in {@link Map} | ||
| * and returns this cached map. | ||
| * Filters out the keys which are already cached and fetches only the not-cached category keys from the CTP project | ||
| * defined in an injected {@link SphereClient} and stores a mapping for every category to id in the cached map of |
Contributor
There was a problem hiding this comment.
Suggested change
| * defined in an injected {@link SphereClient} and stores a mapping for every category to id in the cached map of | |
| * d and stores a mapping for every category to id in the cached map of |
|
|
||
| /** | ||
| * Filters out the keys which are already cached and fetches only the not-cached customer group keys from the CTP | ||
| * project defined in an injected {@link SphereClient} and stores a mapping for every customer group to id in |
Contributor
There was a problem hiding this comment.
Suggested change
| * project defined in an injected {@link SphereClient} and stores a mapping for every customer group to id in | |
| * } and stores a mapping for every customer group to id in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improvement on the caching concept:
Currently, sync classes that doing reference resolution(for example ProductSync) only executes the
cacheKeysToIdsservice methods to cache just for the draft resources (for example products for the ProductSync) but a resource could have other references (for example product type, tax category, state, and many others for ProductSync). And executing/syncing a batch of drafts the library is fetching one resource to find the id of the one reference.We might improve the performance a lot of the library with iterating all references in a resource and collecting all referenced keys in a batch, then fetching together instead of fetching one by one during reference resolution (which is not good on the performance).
Assume a batch has 50 products, and 50 products will have 50 different custom object references, for this we will go 50 times into the commercetools. With the new design, we should aim at going only one time to collect all key to id mapping of the custom object.
Additionally, the BathProcessor concept is generalized as BaseBatchValidator for other sync processes, check the BaseBatchValidator class.
Relevant Issues
#235
Todo