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

Fix GC actor integration #584

Merged
merged 3 commits into from Oct 5, 2021
Merged

Fix GC actor integration #584

merged 3 commits into from Oct 5, 2021

Conversation

evanxg852000
Copy link
Contributor

Description

Closes #573
Fixed GC Actor integration into pipeline

How was this PR tested?

Run indexing

@evanxg852000 evanxg852000 marked this pull request as ready for review September 25, 2021 07:59
Comment on lines 366 to 367
if handlers.publisher.health() != Health::Healthy
&& handlers.garbage_collector.health() == Health::Healthy
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a bug in the indexer logic.

Let me try to describe the problem it is a bit tricky.
.health() is actually mutating something.

It uses a strange contraption in the Progress object that checks if a has_changed is set to true.
At every healthchec this flag is checked, and the actor is in charge of flipping it up until the next iteration of the loop.

Here we call health a second time right after the regular healthcheck call. 95% of the time the actor will not have modified it so that both the publisher and the garbage collector are not healthy.

The fix I added for the MergePlanner is checking for the state of the actors instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why I was:

I resorted to the finalize method of the publisher. checking the health state of the publisher from the supervisor was yielding inconsistent state.

@evanxg852000 evanxg852000 merged commit 0fb049f into main Oct 5, 2021
@evanxg852000 evanxg852000 deleted the fix-573 branch October 5, 2021 09:39
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.

register progress after each file in run_garbage_collect
2 participants