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

Input plugins should retry database creation #7477

Merged
merged 4 commits into from
Oct 18, 2016
Merged

Conversation

e-dard
Copy link
Contributor

@e-dard e-dard commented Oct 18, 2016

Required for all non-trivial PRs

This PR changes how input plugins open. Instead of trying to create a database on open they will instead check if they have successfully created a database when processing data.

@jwilder

Copy link
Contributor

@jwilder jwilder left a comment

Choose a reason for hiding this comment

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

Looks good except it seems like createInternalStorage should be returning an error if it fails.

@@ -397,6 +439,9 @@ func (s *Service) processBatches(batcher *tsdb.PointBatcher) {
for {
select {
case batch := <-batcher.Out():
// Will attempt to create database if not yet created.
s.createInternalStorage()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check for an error being returned?

case batch := <-batcher.Out():
// Will attempt to create database if not yet created.
s.createInternalStorage()
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there is nothing we can do but log the error, it seems cleaner to have createInternalStorage check for an error and log it itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

But then it continues on to try the write which will fail as well and tie up resources. Seems like we should abort the write here since we already know it will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry I see what you mean. OK I'll refactor that.

@e-dard e-dard force-pushed the er-service-create branch 3 times, most recently from 3e2b5a0 to 18dfcf3 Compare October 18, 2016 15:35
@e-dard e-dard force-pushed the er-service-create branch from 18dfcf3 to 168c91c Compare October 18, 2016 15:50
@e-dard e-dard merged commit 4776e8f into master Oct 18, 2016
@e-dard e-dard deleted the er-service-create branch October 18, 2016 17:11
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.

2 participants