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.

Sign up for GitHub

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

Jump to bottom

Add possibility to manual db-cache invalidation #3184

Merged
merged 1 commit into from
Oct 22, 2022
Merged

Add possibility to manual db-cache invalidation #3184

merged 1 commit into from
Oct 22, 2022

Conversation

porunov
Copy link
Member

@porunov porunov commented Aug 26, 2022 •
edited
Loading

This PR adds possibility to manually expire / invalidate db-cache entries.

Related to #3155

Signed-off-by: Oleksandr Porunov [email protected]

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@porunov porunov added this to the Release v1.0.0 milestone Aug 26, 2022
@porunov porunov requested a review from li-boxuan August 26, 2022 19:09
@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Aug 26, 2022
@porunov porunov force-pushed the feature/db-cache-invalidation branch 2 times, most recently from d462ea0 to 432075f Compare August 26, 2022 23:43
@porunov porunov force-pushed the feature/db-cache-invalidation branch from 432075f to 011160e Compare August 31, 2022 21:38
@porunov
Copy link
Member Author

porunov commented Aug 31, 2022

@li-boxuan changed the interface, excluded that previously added section from JanusGraph doc, covered new methods with integration tests (edgeStore invalidation by keys, indexStore invalidation by keys, full invalidation of edgeStore and indexStore), added JavaDoc to CacheInvalidationService.
The only thing I didn't fully check is the first example I added into the JavaDoc of CacheInvalidationService#markKeyAsExpiredInIndexStore(StaticBuffer) but I will double check that example tomorrow.
Other than that, the PR is ready for review

@porunov porunov force-pushed the feature/db-cache-invalidation branch 2 times, most recently from 8d4cdbf to 87d1778 Compare September 1, 2022 00:27
@porunov porunov force-pushed the feature/db-cache-invalidation branch from 87d1778 to 9e77576 Compare September 2, 2022 18:10
li-boxuan
li-boxuan reviewed Sep 11, 2022
View reviewed changes
...-core/src/main/java/org/janusgraph/diskstorage/keycolumnvalue/cache/ExpirationKCVSCache.java
clearExpiredCache(false);
}

private synchronized void clearExpiredCache(boolean withNewPenaltyCountdown) {
Copy link
Member

Choose a reason for hiding this comment

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

@porunov Do you know why we don't clear expired keys right away? I am not sure if I understand why we need to clean them using a dedicated thread.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm I saw your reply here: #3198 (comment) I guess we would create a follow-up issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think we will need to open follow-up PRs for direct invalidation without having expire functionality at all, but I didn't check the complexity of such operations yet

Copy link
Member Author

Choose a reason for hiding this comment

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

@li-boxuan I created a follow-up issue regarding cache here: #3218

@porunov porunov force-pushed the feature/db-cache-invalidation branch from 9e77576 to 64c6239 Compare September 22, 2022 11:46
@porunov
Copy link
Member Author

porunov commented Oct 21, 2022

@li-boxuan would you like to re-review this PR or is it OK to proceed with the lazy consensus?

@li-boxuan
Copy link
Member

@porunov Sorry I don't have much bandwidth recently. I've reviewed it once before anyways, so I am OK to proceed with the lazy consensus.

@porunov
Copy link
Member Author

porunov commented Oct 22, 2022

@porunov Sorry I don't have much bandwidth recently. I've reviewed it once before anyways, so I am OK to proceed with the lazy consensus.

Okay, I think it's safe to proceed with the lazy consensus because this PR adds new functionality and doesn't change logic of existing functionality.

@porunov porunov merged commit 7ac198f into JanusGraph:master Oct 22, 2022
@FlorianHockmann FlorianHockmann mentioned this pull request Jan 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@li-boxuan li-boxuan li-boxuan left review comments

Assignees
No one assigned
Labels
cla: external Externally-managed CLA
Projects
None yet
Milestone
Release v1.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@porunov @li-boxuan @janusgraph-bot