Skip to content

Add caching for versions infos - #900

Open
LiubovDidkivska wants to merge 3 commits into
masterfrom
task/olpedge-1606_versions_api_2__
Open

Add caching for versions infos#900
LiubovDidkivska wants to merge 3 commits into
masterfrom
task/olpedge-1606_versions_api_2__

Conversation

@LiubovDidkivska

@LiubovDidkivskaLiubovDidkivska commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

Add implementation to put/get versions
list to catalog client cache.
Implement versions infos cache tests.

Relates-To: OLPEDGE-2080

Signed-off-by: Liubov Didkivska ext-liubov.didkivska@here.com

@codecov

codecovBot commented Jun 8, 2020

Copy link
Copy Markdown

Codecov Report

Merging #900 into master will increase coverage by 0.0%.
The diff coverage is 87.8%.

Impacted file tree graph

@@ Coverage Diff @@## master #900 +/- ##
======================================
Coverage 81.2% 81.2% ======================================
Files 303 305 +2 Lines 10575 10620 +45 ======================================
+ Hits 8591 8628 +37 - Misses 1984 1992 +8 
Impacted FilesCoverage Δ
...ice-read/src/repositories/CatalogCacheRepository.h100.0% <ø> (ø)
...rc/generated/serializer/VersionInfosSerializer.cpp64.3% <64.3%> (ø)
...e-read/src/generated/parser/JsonParserCacheValue.h100.0% <100.0%> (ø)
...e-read/src/repositories/CatalogCacheRepository.cpp100.0% <100.0%> (ø)
...ude/olp/dataservice/read/model/VersionDependency.h57.1% <0.0%> (-42.9%)⬇️
...d/include/olp/dataservice/read/model/VersionInfo.h100.0% <0.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd2d656...0ef9d58. Read the comment docs.

@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch 7 times, most recently from db5e6c8 to ba94dd9CompareJune 9, 2020 08:17
@LiubovDidkivskaLiubovDidkivska changed the title Add metadata api to get versions listAdd versions list implementationJun 9, 2020
@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from 610f818 to 0d299bdCompareJune 9, 2020 09:52
Comment threadolp-cpp-sdk-dataservice-read/include/olp/dataservice/read/CatalogClient.h Outdated
Comment threadolp-cpp-sdk-dataservice-read/src/CatalogClientImpl.cpp Outdated
Comment on lines 139 to 149

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder why do we need then copy parameters on CatalogRepository::GetVersionsList() if it is sync? Can we move towards pass by reference to save copies?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Could we do this in separate pull request and make this changes also in other clients?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Considering our current backlog I dont think that will be any time soon. Ill add a ticket to the backlog for this but i would prefer if we can already do it in your PR as we also did it in #891.
But please note that this is not valid for all methods. For example CatalogClient::GetCatalog() could still use it as it does not have any version relation.

@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from 019bbda to 13c5256CompareJune 9, 2020 10:48
@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from c50c5ff to 0f27430CompareJune 9, 2020 16:02

@andescuandescu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tests missing!

Comment on lines 139 to 149

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Considering our current backlog I dont think that will be any time soon. Ill add a ticket to the backlog for this but i would prefer if we can already do it in your PR as we also did it in #891.
But please note that this is not valid for all methods. For example CatalogClient::GetCatalog() could still use it as it does not have any version relation.

Comment threadolp-cpp-sdk-dataservice-read/src/CatalogClientImpl.cpp Outdated
Comment threadolp-cpp-sdk-dataservice-read/src/repositories/CatalogCacheRepository.cpp Outdated
Comment threadolp-cpp-sdk-dataservice-read/src/repositories/CatalogCacheRepository.cpp Outdated
Comment threadolp-cpp-sdk-dataservice-read/src/repositories/CatalogCacheRepository.cpp Outdated
@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from 0f27430 to e327e4eCompareJune 10, 2020 09:02
@LiubovDidkivskaLiubovDidkivska changed the title Add versions list implementationAdd caching for versions infosJun 10, 2020
@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from e327e4e to 61e3967CompareJune 10, 2020 09:05
Comment threadolp-cpp-sdk-dataservice-read/src/repositories/CatalogCacheRepository.cpp Outdated
Comment threadolp-cpp-sdk-dataservice-read/src/repositories/CatalogCacheRepository.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please also add a test case for start_version = -1 as specified by the documentation:

startVersion
The beginning of the range of versions you want to get (exclusive). By convention -1 indicates the initial version before the first publication. After the first publication, the catalog version is 0.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Test will be added to CatalogRepositoryTest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But why in CatalogRepositoryTest as this will have direct implication upon the key generation, which needs to be tested that it works correctly, means that not by mistake the -1 startVersion is converted to 0 as we do not support -1 while converting to string.

@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from 61e3967 to d18a5f0CompareJune 10, 2020 11:15
Comment threadolp-cpp-sdk-dataservice-read/src/generated/parser/JsonParserCacheValue.h Outdated
Liubov Didkivska added 3 commits June 10, 2020 17:24
Add implementation to put/get versions
list to catalog client cache.
Implement versions infos cache tests.
Relates-To: OLPEDGE-1606
Signed-off-by: Liubov Didkivska <ext-liubov.didkivska@here.com>
Add implementation to put/get versions
list to catalog client cache.
Implement versions infos cache tests.
Relates-To: OLPEDGE-1606
Signed-off-by: Liubov Didkivska <ext-liubov.didkivska@here.com>
Add implementation to put/get versions
list to catalog client cache.
Implement versions infos cache tests.
Relates-To: OLPEDGE-1606
Signed-off-by: Liubov Didkivska <ext-liubov.didkivska@here.com>
@LiubovDidkivska
LiubovDidkivskaforce-pushed the task/olpedge-1606_versions_api_2__ branch from e3b5dbd to 0ef9d58CompareJune 10, 2020 14:24
Sign up for freeto 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.

2 participants

@LiubovDidkivska@andescu