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

LUCENE-9298: Improve RAM accounting in BufferedUpdates when deleted doc IDs and terms are cleared #1389

Merged
merged 8 commits into from
Apr 10, 2020

Conversation

bringyou
Copy link
Contributor

@bringyou bringyou commented Mar 30, 2020

the method clearDeletedDocIds in BufferedUpdates.java has a bug, it can't reset bytesUsed correctly.

void clearDeletedDocIds() {
  deleteDocIDs.clear();
  bytesUsed.addAndGet(-deleteDocIDs.size() * BufferedUpdates.BYTES_PER_DEL_DOCID);
}

this PR will fix it.

@bringyou
Copy link
Contributor Author

bringyou commented Apr 1, 2020

@s1monw sorry to bother, but can you take a look?

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Would you mind adding a small test for this issue? Thanks @bringyou!

bringyou and others added 2 commits April 9, 2020 00:08
2. decrease bytesUsed when clearDeleteTerms;
3. fix the clearDeletedDocIds
@bringyou
Copy link
Contributor Author

bringyou commented Apr 8, 2020

Change looks good to me. Would you mind adding a small test for this issue? Thanks @bringyou!

sorry for the delay~ add a test for BufferedUpdates and change a bit more code, please take another look @dnhatn

Copy link
Member

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

the test looks good. I like the change, I left one comment

@bringyou bringyou requested a review from s1monw April 9, 2020 02:56
Copy link
Member

@s1monw s1monw 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 left one suggestions that we should merge in. Was my fault.

@s1monw
Copy link
Member

s1monw commented Apr 9, 2020

I ran tests and we are ready to go. Please add a changes entry like this:

diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index b5bc71cb759..2daa8f3c71f 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -238,6 +238,9 @@ Improvements
 * LUCENE-9171: QueryBuilder can now use BoostAttributes on input token streams to selectively
   boost particular terms or synonyms in parsed queries. (Alessandro Benedetti, Alan Woodward)
 
+* LUCENE-9298: Improve RAM accounting in BufferedUpdates when deleted doc IDs and terms are cleared.
+  (Your Name, Simon Willnauer)
+
 Optimizations
 ---------------------

@bringyou bringyou changed the title LUCENE-9298: fix clearDeletedDocIds in BufferedUpdates LUCENE-9298: Improve RAM accounting in BufferedUpdates when deleted doc IDs and terms are cleared Apr 10, 2020
@bringyou
Copy link
Contributor Author

@s1monw thanks for the reviews, I add the changelogs and edit issue's title

@s1monw s1monw merged commit 2935186 into apache:master Apr 10, 2020
@s1monw
Copy link
Member

s1monw commented Apr 10, 2020

thanks!!

s1monw pushed a commit that referenced this pull request Apr 10, 2020
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.

3 participants