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

Hinted handoff should not process inactive nodes #4339

Closed
wants to merge 3 commits into from

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Oct 6, 2015

It is possible that a node is dropped, while data is still pending in the HH queues for that node. If this happens, the HH processor should skip those nodes, or the system continually complains about unreachable nodes.

Data deletion is still explicitly left to the purge system, though an end-user can always remove HH data by hand if necessary.

This issue is not very serious, but can cause confusion and results in connection errors in the logs.

@otoolep otoolep force-pushed the hh_drop_server branch 2 times, most recently from a59c3cb to ad29b88 Compare October 6, 2015 05:05
@otoolep otoolep changed the title Some initial work Hinted handoff should not process inactive nodes Oct 6, 2015
It is possible that a node is dropped, while data is still pending in
the HH queues for that node. If this happens, the HH processor should
skip those nodes, or the system continually complains about unreachable
nodes.

Data deletion is still explicitly left the purge system.
@otoolep
Copy link
Contributor Author

otoolep commented Oct 6, 2015

@jwilder @corylanou

for nodeID, q := range p.queues {
// Only process queues for active nodes.
var activeQueues map[uint64]*queue
if p.MetaProvider == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this actually happen? There is always a meta store (remote or local) on each server right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make the Provider optional, so if you want all the complexity, you add it. Otherwise the Provider will just handle the nodes it got. It also makes testing cleaner. I don't feel strongly about this.

@corylanou
Copy link
Contributor

Just a couple questions, but still gets a +1.

res := make(chan error, len(p.queues))
for nodeID, q := range p.queues {
// Only process queues for active nodes.
var activeQueues map[uint64]*queue
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pulling this activeQueues logic into a func activeQueues() help keep the Process function simpler.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 6, 2015

Thanks for the reviews @jwilder @corylanou

Now that I see @jwilder originally considered that this functionality would be part of "purge", let me think about adding it there. It might be cleaner.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 8, 2015

I still plan to fix this, and leaving the deletion of data to happen within purge makes most sense. However, I still think it is best that data is not deleted until it's 7 days old regardless of whether the node is active or not. This protects the user in the event that he drops the wrong ID. It gives him time to get to that HH data, as opposed to anything that forces the system to drop any HH data immediately on DROP SERVER. The user can still manually delete any HH files if he wishes to reclaim the space earlier.

This seems more in-line with the principle of DROP SERVER never directly dropping data. Agreed?

@jwilder
Copy link
Contributor

jwilder commented Oct 8, 2015

Sounds good.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 8, 2015

Closing favour of #4377

@otoolep otoolep closed this Oct 8, 2015
@otoolep otoolep deleted the hh_drop_server branch October 28, 2015 20:07
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