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.

Sign up for GitHub

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

Jump to bottom

Shard precreation service #2878

Merged
merged 8 commits into from
Jun 10, 2015
Merged

Shard precreation service #2878

merged 8 commits into from
Jun 10, 2015

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Jun 10, 2015

See file notes.md for full a full description.

Fixes #2735

@otoolep otoolep changed the title Pre-create shards Shard precreation service Jun 10, 2015
@otoolep otoolep force-pushed the shard_precreation branch 4 times, most recently from 1bc10eb to 049c84d Compare June 10, 2015 18:56
@jwilder
Copy link
Contributor

jwilder commented Jun 10, 2015

I think this should just be part of the current retention package. That already handles shard and shard group deletion so adding creation there too seems natural to me.

@otoolep
Copy link
Contributor Author

otoolep commented Jun 10, 2015

I'm not sure @jwilder -- they are different things. One is about enforcing retention, the other is about optimizing the system to prevent shard creation on writes, with possibly lots of action on the consensus system. Apart from the fact that they both manipulate shard groups, they don't have much in else in common, right?

@otoolep
Copy link
Contributor Author

otoolep commented Jun 10, 2015

Apart from the conceptual difference (in my mind) one could turn off shard precreation and the system would work perfectly fine (apart from the impact on write performance at certain times). But disable retention and the system will eat up more and more disk.

Also, retention is a feature, our customers are aware of. This particular service is an implementation detail, albeit an important one. I do consider them quite separate.

@otoolep otoolep force-pushed the shard_precreation branch 3 times, most recently from e1c17f9 to 6155078 Compare June 10, 2015 19:59
@otoolep
Copy link
Contributor Author

otoolep commented Jun 10, 2015

Ready for merging.

@pauldix @corylanou

@jwilder
Copy link
Contributor

jwilder commented Jun 10, 2015

They both are features and issues related to retention policies though. A shard pre-creator doesn't makes sense on it's own without a retention policy which is why I think it belongs with a retention policy related package. Shard groups and shards are implementation details of retention policies.

This pre-creation could be a separate service, but I don't think it needs to be since the current retention service has virtually the same dependencies as this shard_precreation service. If it's disabled, don't start that goroutine.

It's up to you but I don't think this structure makes sense. Moving it to retention will result in less code and make it more clear who is responsible for shard/shard group lifecycle management vs having it spread out across multiple packages.

jwilder
jwilder reviewed Jun 10, 2015
View reviewed changes
services/shard_precreation/service.go

for {
select {
case <-time.After(s.checkInterval):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a time.Ticker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, and that would be fine, but we don't need that fine a control over when shard precreation runs. A small jitter is a not big deal, since iterations of this loop only happen on the order of minutes. It seems in this case the less code is nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change this, don't feel strongly about it in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion on it. Just noted that it's slightly different than how the other services do this so might raise questions as to why in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, good enough. I think this terse form gives the system exactly what it needs so will leave it for now.

@otoolep
Copy link
Contributor Author

otoolep commented Jun 10, 2015

Thanks @jwilder -- good points. Unless I get push back from other members of the team, I'm going to go with the existing structure however, since I prefer decomposing the system like this. Of course, if it doesn't work, that is a different issue but I think we both agree both approaches work and are understandable.

benbjohnson
benbjohnson reviewed Jun 10, 2015
View reviewed changes
services/shard_precreation/service.go
}
})
return numCreated, nil
}
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 this should be moved into meta.Store.Precreate() and the service should just call that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair, and will help with future consolidation if we go there. Let me make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, thinking about it a bit more, I think that might be going to a bit too far. I like the idea of using a simpler name, and moving the function into the meta package, but I'm going to keep it focused on shard creation for now. I've done some of the coding, and it doesn't feel right to generalize it to that extent -- not yet anyway.

@benbjohnson
Copy link
Contributor

I don't have a strong opinion about consolidation although if we keep it in a separate package then I'd suggest just making it precreator. It can call meta.Store.Precreate() and the metastore can figure out how to handle it.

If we have other types of data that need to be precreated (besides shard groups) then those can get lumped in there later.

@otoolep
Copy link
Contributor Author

otoolep commented Jun 10, 2015

Thanks for the feedback @jwilder and @benbjohnson -- precreator is a better name. Let me make those final changes.

@otoolep
Copy link
Contributor Author

otoolep commented Jun 10, 2015

Final changes made. No logic changes, just naming and pushed shard creation logic into the store. While the service remains dedicated to shards, I still prefer a shorter name for the service, so went that change.

I plan to merge this today, so any final feedback is welcome.

@benbjohnson
Copy link
Contributor

lgtm

otoolep added a commit that referenced this pull request Jun 10, 2015
@otoolep
Merge pull request #2878 from influxdb/shard_precreation
01f2d7d 
Shard precreation service
@otoolep otoolep merged commit 01f2d7d into master Jun 10, 2015
@otoolep otoolep deleted the shard_precreation branch June 10, 2015 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@otoolep @jwilder @benbjohnson