make a dump command #1909

rothrock · 2015-03-10T23:33:20Z

added a 'dump' command to the cli.
I still need to see about getting the name of the db and the retention policy.
The idea is that the dump command outputs something that can be played back as input.

corylanou · 2015-03-12T20:27:42Z

I think this is looking good.

I am wondering though if we shouldn't create a "dump" endpoint from the http handlers. This would allow you to dump via a curl (very useful), and then the cli becomes very dumb about it and just has to hit the http endpoint.

My suggestion would be to move the logic to create the "dump" struct into an http endpoint, and then hit the client from the cli, that hits the endpoint.

Reminder to update the CHANGELOG.md as well when finished.

rothrock · 2015-03-12T20:36:23Z

OK, I think I follow it now. Sure. That would be useful.

Issue: 1909 move dump cmd to an http endpoint. Add dump cmd on client to call the endpoint.

and update changelog

corylanou · 2015-03-13T23:16:03Z

client/influxdb.go

@@ -145,6 +145,27 @@ func (c *Client) Ping() (time.Duration, string, error) {
 	return time.Since(now), version, nil
 }

+func (c *Client) Dump(db string) (*http.Response, error) {


I think we should return a io.ReadCloser so that anyone else consuming our library wouldn't have to add the http package if they wanted to declare a variable to receive the response.

OK, but then others consuming our library will need to include the io package, right?

Potentially, yes, but it only exposes what you want to expose. Returning the entire http response is way to much to return. Also, it's very idiomatic in Go to pass around readers/writers.

+1 for returning io.ReadCloser. It also doesn't tie the Client.Dump() interface to HTTP. Maybe we'll add a named pipes transport in the future. ;)

corylanou · 2015-03-13T23:41:57Z

Looking good. Some more comments for discussion. We also need to add handler tests, and integration tests.

rothrock · 2015-03-17T22:51:16Z

@toddboom putting in a sleep "fixes" it. That's no good.

corylanou · 2015-03-17T22:52:25Z

httpd/handler_test.go

+		t.Fatalf("unexpected status for get: %d", status)
+	}
+
+	time.Sleep(500 * time.Millisecond) // Shouldn't committed data be readable?


Writing data is not committed data. When you do a write, that means that we accepted the message, and are sending it across brokers to write it and mark as committed. The better practice than sleep is to query and wait.

We do this in other tests here: https://github.com/influxdb/influxdb/blob/master/cmd/influxd/server_integration_test.go#L210

I wouldn't change it for now, but in general, this can lead to a racy test, as well as it artificially inflates our test suite, which adds up over time.

rothrock · 2015-03-17T22:56:09Z

That's counterintuitive. I notice other sleeps in the test code. I also noticed tests that never verify that the data they wrote made it to the db.

rothrock · 2015-03-17T22:58:08Z

our docs say:

Response
Once InfluxDB has accepted this data and safely persisted it to disk, it responds with HTTP 200 OK.

rothrock · 2015-03-17T23:04:05Z

@corylanou @toddboom Can I merge this?

corylanou · 2015-03-17T23:05:50Z

When we are done reviewing a PR, we typically give it a +1 or 🚢, etc. when it is ready to merge. I think there are reviews going on yet. I know I'm not done yet.

rothrock · 2015-03-17T23:11:07Z

Sure, @corylanou, take your time. We've never really discussed procedures around here.

corylanou · 2015-03-17T23:12:43Z

client/influxdb.go

+	if err != nil {
+		return nil, err
+	}
+	return resp.Body, nil


We probably need to check the status code as well to see if we got an ok response, if not, return some type of error.

We do something like that here:

https://github.com/influxdb/influxdb/blob/master/client/influxdb.go#L121

I was looking to the Query handler for inspiration when I coded the Dump handler.

OK. we don't check http response codes for query or ping.

The cited example looks to be a mistake. It seem like the err returned on line 123 will always be nil. In the unlikely case that it is non-nil, the error returned doesn't speak to the problem the http client encountered. The error returned is in the context of json decoding.

OK, now it returns

return resp.Body, fmt.Errorf("HTTP Protocol error %d", resp.StatusCode)

otoolep · 2015-03-17T23:19:21Z

On Tue, Mar 17, 2015 at 3:58 PM, Joseph Rothrock notifications@github.com
wrote:

our docs say:

Response
Once InfluxDB has accepted this data and safely persisted it to disk, it
responds with HTTP 200 OK.

Note that it doesn't say anything about the data being indexed in the
time-series database. Safely to disk in this case means sent to the
brokers. The assumption is the Broker are streaming it to the topic logs,
which are on disk.This may need some tweaking with the new Raft work, but
we should be able to build a system that can give this guarantee.

—

Reply to this email directly or view it on GitHub
#1909 (comment).

rothrock · 2015-03-17T23:22:19Z

@otoolep that's great that safely-to-disk means "written to a broker," but that's not intuitive.
Also, I don't see the set-and-test pattern in the test coverage that such behavior would imply.

corylanou · 2015-03-17T23:24:43Z

cmd/influx/main.go

+	if err != nil {
+		fmt.Printf("Dump failed. %s\n", err)
+	} else {
+		scanner := bufio.NewScanner(response)


I think you can do:

_, err := io.Copy(os.Stdout, response)

See this example
http://play.golang.org/p/lK2kFDJUw0

The response is line-oriented, so this felt like a natural way to do it.
The code works.

I will change it if you insist.

corylanou · 2015-03-17T23:49:48Z

@rothrock ok, done with my review. Let me know if you have any questions. Super excited to see this in production! Thanks!

toddboom · 2015-03-17T23:51:59Z

@rothrock For propagation delays, I think something like this is the preferred solution (whenever possible) going forward:
https://github.com/influxdb/influxdb/blob/master/cmd/influxd/server_integration_test.go#L210

That way we can timebox our tests to some upper bound, but keep them responsive.

pauldix · 2015-03-18T20:56:51Z

@otoolep if that's what the docs say about writes, they should be updated as they're inaccurate. Writes will return a 200 once a quorum of the raft cluster has verified the write.

However, at that stage the write is only in memory. They haven't necessarily fsync'd the write to disk. This means there is a potential for data loss on a confirmed write if you have a catastrophic failure among the brokers before they manage to sync the write to disk.

This is fine, but it should be accurate in the documentation.

We'll probably add some configuration option later to force the raft servers to fsync before every commit to the raft log, but that's not the default behavior.

otoolep · 2015-03-18T21:03:45Z

Docs clarified @pauldix

rename dump flag put an error response body if json marshalling breaks. detect and respond to http error codes in the client.

pauldix · 2015-03-19T01:59:36Z

@otoolep awesome, thanks!

toddboom · 2015-03-19T06:03:48Z

@corylanou how's she looking now? 👍 from me to merge if you think it's gtg.

corylanou · 2015-03-19T12:31:24Z

+1 🚢

Add a `dump` command for exporting data.

…rated Switch buckets ui to use generated client

make a dump command

3193e21

rothrock added the 2 - Working label Mar 10, 2015

turn a 'Row' into distinct points

12760b9

rothrock added 2 commits March 12, 2015 17:14

move to http handler

6f86cac

Dump cmd

4aabcd9

Issue: 1909 move dump cmd to an http endpoint. Add dump cmd on client to call the endpoint.

rothrock self-assigned this Mar 13, 2015

rothrock added 2 commits March 13, 2015 16:07

Merge branch 'master' of github.com:influxdb/influxdb into export

4aa47db

and update changelog

remove commented line

a342a1e

corylanou reviewed Mar 13, 2015
View reviewed changes

beckettsean modified the milestones: 0.9.0, Next Release Mar 14, 2015

toddboom modified the milestones: Next Point Release, 0.9.0 Mar 14, 2015

rothrock added 2 commits March 16, 2015 15:31

tweaking in response to various PR comments

d742068

Merge branch 'master' of github.com:influxdb/influxdb into export

53d9963

sleep hack

91014de

corylanou reviewed Mar 17, 2015
View reviewed changes

dump cmd

21fd2e2

rename dump flag put an error response body if json marshalling breaks. detect and respond to http error codes in the client.

toddboom added review and removed 2 - Working labels Mar 18, 2015

Final cleanups based on review feedback.

8425c0c

toddboom added a commit that referenced this pull request Mar 19, 2015

Merge pull request #1909 from influxdb/export

e2391d9

Add a `dump` command for exporting data.

toddboom merged commit e2391d9 into master Mar 19, 2015

toddboom deleted the export branch March 19, 2015 22:07

mark-rushakoff pushed a commit that referenced this pull request Jan 11, 2019

Merge pull request #1909 from influxdata/chore/switch-buckets-ui-gene…

f5a035b

…rated Switch buckets ui to use generated client

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make a dump command #1909

make a dump command #1909

rothrock commented Mar 10, 2015

corylanou commented Mar 12, 2015

rothrock commented Mar 12, 2015

corylanou Mar 13, 2015

rothrock Mar 14, 2015

corylanou Mar 14, 2015

benbjohnson Mar 14, 2015

corylanou commented Mar 13, 2015

rothrock commented Mar 17, 2015

corylanou Mar 17, 2015

rothrock commented Mar 17, 2015

rothrock commented Mar 17, 2015

rothrock commented Mar 17, 2015

corylanou commented Mar 17, 2015

rothrock commented Mar 17, 2015

corylanou Mar 17, 2015

rothrock Mar 17, 2015

rothrock Mar 18, 2015

rothrock Mar 18, 2015

otoolep commented Mar 17, 2015

rothrock commented Mar 17, 2015

corylanou Mar 17, 2015

rothrock Mar 18, 2015

corylanou commented Mar 17, 2015

toddboom commented Mar 17, 2015

pauldix commented Mar 18, 2015

otoolep commented Mar 18, 2015

pauldix commented Mar 19, 2015

toddboom commented Mar 19, 2015

corylanou commented Mar 19, 2015

make a dump command #1909

make a dump command #1909

Conversation

rothrock commented Mar 10, 2015

corylanou commented Mar 12, 2015

rothrock commented Mar 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corylanou commented Mar 13, 2015

rothrock commented Mar 17, 2015

Choose a reason for hiding this comment

rothrock commented Mar 17, 2015

rothrock commented Mar 17, 2015

rothrock commented Mar 17, 2015

corylanou commented Mar 17, 2015

rothrock commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otoolep commented Mar 17, 2015

rothrock commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corylanou commented Mar 17, 2015

toddboom commented Mar 17, 2015

pauldix commented Mar 18, 2015

otoolep commented Mar 18, 2015

pauldix commented Mar 19, 2015

toddboom commented Mar 19, 2015

corylanou commented Mar 19, 2015