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

GH-4954 LMDB: Fix GC and record counting for deletions #4955

Merged

Conversation

kenwenzel
Copy link
Contributor

@kenwenzel kenwenzel commented Apr 17, 2024

Ensures that GC is also working if triple store needs to grow and that the correct count is returned for statement removals.

GitHub issue resolved: #4954

Briefly describe the changes proposed in this PR:

Tests removed values after the triple store's transaction is committed and correctly calls handler in removeStatement methods.

PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

@kenwenzel kenwenzel force-pushed the GH-4954-lmdb-gc-record-count branch from b836747 to 80e8c60 Compare April 18, 2024 06:10
@hmottestad
Copy link
Contributor

@kenwenzel You know you have committer rights now so you don't have to use your fork anymore.

@kenwenzel
Copy link
Contributor Author

kenwenzel commented Apr 18, 2024

@kenwenzel You know you have committer rights now so you don't have to use your fork anymore.

Thank you for the.hint. Do you prefer the creation of branches in this repo rather than using a/my fork?

@hmottestad
Copy link
Contributor

The failing integration tests: It's a test that is marked as disabled, but has somehow started to run again. Not sure how that happened.

Ensures that GC is also working if triple store needs to grow
and that the correct count is returned for statement removals.
@kenwenzel kenwenzel force-pushed the GH-4954-lmdb-gc-record-count branch from 80e8c60 to b784f07 Compare April 22, 2024 08:16
@hmottestad
Copy link
Contributor

@kenwenzel You know you have committer rights now so you don't have to use your fork anymore.

Thank you for the.hint. Do you prefer the creation of branches in this repo rather than using a/my fork?

Would be nice if you used the RDF4J repo. Nice thing is that it allows others to work on your branch, though that's probably not going to happen very often.

@kenwenzel
Copy link
Contributor Author

@hmottestad Should I merge this on my own?

@hmottestad
Copy link
Contributor

I prefer if you request a code review. It doesn't have to be me doing the code review, can also be someone else who is active on the project, at the moment it's mostly Jerven who is active. You can request a review from either one of us, and if you get an "approval" then feel free to merge it.

If you don't get a response to your review request ping me and if you still don't hear anything after a few more days then feel free to do a "self review" and merge. Unless you feel that your changes are very big, a fundamental change or you have a funky solution to something that you want feedback on.

@hmottestad hmottestad self-requested a review April 27, 2024 07:07
Copy link
Contributor

@hmottestad hmottestad left a comment

Choose a reason for hiding this comment

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

Can't say I understand much of what's going on. LGTM.

@kenwenzel kenwenzel merged commit ccf6250 into eclipse-rdf4j:develop May 15, 2024
9 checks passed
@kenwenzel kenwenzel deleted the GH-4954-lmdb-gc-record-count branch May 15, 2024 09:18
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