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

Jump to bottom

Add possibility to remove composite index entries #3022

Merged
merged 1 commit into from May 5, 2022
Merged

Add possibility to remove composite index entries #3022

merged 1 commit into from May 5, 2022

Conversation

porunov
Copy link
Member

@porunov porunov commented Apr 27, 2022

With this change it will be possible to remove stale composite index entries from the graph.

Current limitations:

  • Current approach didn't cover Mixed indices. I think it's possible to add this feature in the future PRs but I didn't dig into mixed index direct record changes yet.
  • It's more challenging to change Edge composite index record but still possible in some cases.
  • Vertex centric indices were not covered. I guess it's more challenging but I didn't dig into changing entries of vertex centric indices yet. (not sure if that's needed at all)

Related to #1099

Signed-off-by: Oleksandr Porunov alexandr.porunov@gmail.com

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 Apr 27, 2022
@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Apr 27, 2022
li-boxuan
li-boxuan reviewed Apr 27, 2022
View reviewed changes
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

I only took a quick glance but this is great! Do you mind splitting it into two commits/PRs? Now it is a bit difficult to distinguish new changes from refactoring.

docs/advanced-topics/stale-index.md Outdated Show resolved Hide resolved
@porunov
Copy link
Member Author

porunov commented Apr 27, 2022

Do you mind splitting it into two commits/PRs?

This functionality requires code refactoring due to many private static classes / methods. I can open a refactoring PR tomorrow and when it is merged I can rebase this PR. If so, the amount of changes will be much smaller in this PR and easier to understand

@li-boxuan
Copy link
Member

If so, the amount of changes will be much smaller in this PR and easier to understand

Yeah that would be great!

porunov added a commit to porunov/janusgraph that referenced this pull request Apr 27, 2022
@porunov
Refactor index updates to prepare for JanusGraph#3022
42e49f5 
Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
@porunov porunov mentioned this pull request Apr 27, 2022
Merged
9 tasks
@porunov
Copy link
Member Author

porunov commented Apr 27, 2022

Refactoring PR #3024

porunov added a commit that referenced this pull request Apr 28, 2022
@porunov
Refactor index updates to prepare for #3022
085a5a5 
Related to #1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
@porunov porunov requested a review from li-boxuan April 28, 2022 09:12
@porunov
Copy link
Member Author

porunov commented Apr 28, 2022

@li-boxuan the PR has been rebased

li-boxuan
li-boxuan reviewed May 3, 2022
View reviewed changes
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. I am very busy recently and cannot spend much time on JanusGraph. My comments are mostly related to user experience.

docs/advanced-topics/stale-index.md Outdated Show resolved Hide resolved
docs/advanced-topics/stale-index.md Outdated Show resolved Hide resolved
@porunov porunov requested a review from li-boxuan May 5, 2022 07:13
li-boxuan
li-boxuan approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

Looks great, I just have some minor comments

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/util/StaleIndexRecordUtil.java Outdated Show resolved Hide resolved
janusgraph-core/src/main/java/org/janusgraph/graphdb/database/util/StaleIndexRecordUtil.java Show resolved Hide resolved
janusgraph-core/src/main/java/org/janusgraph/graphdb/database/util/StaleIndexRecordUtil.java Outdated Show resolved Hide resolved
janusgraph-core/src/main/java/org/janusgraph/graphdb/database/util/StaleIndexRecordUtil.java Outdated Show resolved Hide resolved
@porunov
Add possibility to remove composite index entries
d26085b 
With this change it will be possible to remove stale composite index entries from the graph

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
li-boxuan
li-boxuan approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@porunov porunov merged commit deb1f20 into JanusGraph:master May 5, 2022
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 18, 2022
@porunov
Add possibility to remove mixed index entries
7728b38 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 18, 2022
@porunov
Add possibility to remove mixed index entries
ce8a56a 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
@porunov porunov mentioned this pull request Nov 18, 2022
Merged
9 tasks
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 18, 2022
@porunov
Add possibility to remove mixed index entries
cc50720 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 19, 2022
@porunov
Add possibility to remove mixed index entries
ac9f445 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 19, 2022
@porunov
Add possibility to remove mixed index entries
95121dd 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 19, 2022
@porunov
Add possibility to remove mixed index entries
87155b6 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 20, 2022
@porunov
Add possibility to remove mixed index entries
334b5c8 
Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 20, 2022
@porunov
Add possibility to remove stale mixed index entries for given vertice…
208a6a2 
…s or edges

Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 20, 2022
@porunov
Add possibility to remove stale mixed index entries for given vertice…
3995a54 
…s or edges

Continuation of JanusGraph#3022

Related to JanusGraph#1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit that referenced this pull request Nov 21, 2022
@porunov
Add possibility to remove stale mixed index entries for given vertice…
02dba0c 
…s or edges

Continuation of #3022

Related to #1099

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 21, 2022
@porunov
Test stale composite index force record removal works with indexOnly …
93c45de 
…constraint

Related to JanusGraph#1099 and JanusGraph#3022

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this pull request Nov 21, 2022
@porunov
Test stale composite index force record removal works with indexOnly …
a7e7b10 
…constraint

Related to JanusGraph#1099 and JanusGraph#3022

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
This was referenced Nov 21, 2022
Merged
Open
porunov added a commit that referenced this pull request Nov 21, 2022
@porunov
Test stale composite index force record removal works with indexOnly …
6549949 
…constraint

Related to #1099 and #3022

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
li-boxuan
li-boxuan reviewed Apr 21, 2023
View reviewed changes
docs/advanced-topics/stale-index.md
@@ -0,0 +1,47 @@
# Permanent stale index inconsistency
Copy link
Member

Choose a reason for hiding this comment

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

@porunov It seems you forgot to add this doc to the nav config. As a result, this page won't be visible to users.

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 approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@porunov @li-boxuan @janusgraph-bot