Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Trigger ExtentDownEvent when a store is in GoingDown state #148

Merged
merged 6 commits into from
Apr 20, 2017

Conversation

venkat1109
Copy link
Contributor

This patch contains controller side changes to handle graceful deployment of stores. Specifically, the controller triggers sealing of an extent when a store is in GoingDown state (which will be case when a host is about to go down for deployment).

@venkat1109 venkat1109 self-assigned this Apr 13, 2017
@@ -109,6 +109,13 @@ func isInputGoingDown(context *Context, hostID string) bool {
return state == dfddHostStateGoingDown
}

// isStoreGoingDown returns true if the specified store host
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just make this isHostGoingDown(context *Context, sName, hostID string) rather than having separate ones for different roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah.. dfdd does just that.. I will get rid of this.

@@ -143,11 +150,20 @@ func areExtentStoresHealthy(context *Context, extent *m.DestinationExtent) bool
}

for _, h := range storeIDs {

status := "up"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can make this up, down and goingDown an enum as well and define it as a const the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is already a enum within dfdd for this. the right thing to do is to add a String() method to that enum. Let me refactor the code a bit to make it cleaner.

@aravindvs
Copy link
Contributor

One general question on dfdd for stores for my enlightenment. IIUC host "goingDown" event for storehosts is the same as host "down" (removed) event right (i.e, StoreHostFailedEvent)? If that is the case, in the handleHostGoingDown() event, if service == storehost then should we just call handleHostRemovedEvent() and not worry about doing anything else there that way we don't need to create additional StoreFailed event there (i am asking because i like the code now that the failed events are created in a single place which makes the code easier to understand)?

@venkat1109
Copy link
Contributor Author

Yeah, I didn't do a good job of thinking through this carefully, I focused on keeping the changes very minimal from the existing code (specifically, the way hostFailures are detected through rpm).
My long term plan here is to only rely on DFDD for failures and not even look at RPM. So, this code will get even simpler. As for treating StoreHostGoingDown as StoreDown, you are right conceptually, that can be done.

Let me front load some of the things I said above and cleanup this code.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 68.936% when pulling 44e6040 on storestatus into 18499e6 on master.

@venkat1109 venkat1109 force-pushed the storestatus branch 2 times, most recently from 762a93e to 22dce2a Compare April 17, 2017 21:06
@aravindvs
Copy link
Contributor

Looks good now. Saw your comment on the storeGoingDown event vs normal storedown event so we are going to go with a storeFailed event here, which is ok.

As long as the UTs pass, I am good with this.

Copy link
Contributor

@aravindvs aravindvs left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up.. feel free to get it in after the UTs pass..

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 68.595% when pulling 1dbd0b2 on storestatus into c1a8a56 on master.

@venkat1109 venkat1109 merged commit d21c480 into master Apr 20, 2017
@venkat1109 venkat1109 deleted the storestatus branch April 20, 2017 22:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants