Replace NewScanner to NewReader to fix bug #8986 #8999

lets00 · 2017-10-23T19:54:50Z

Replace NewScanner to NewReader to fix bug #8986

Rebased/mergable
Tests pass
CHANGELOG.md updated
Sign CLA (if not already signed)

e-dard

Thanks for the PR, we always appreciate community contributions. I have some suggested feedback. Let me know if you have any questions. Thanks.

e-dard · 2017-10-24T10:08:39Z

CHANGELOG.md

@@ -82,6 +82,7 @@
 - [#8822](https://github.com/influxdata/influxdb/issues/8822): Fix data dropped incorrectly during compaction
 - [#8780](https://github.com/influxdata/influxdb/issues/8780): Prevent deadlock during collectd, graphite, opentsdb, and udp shutdown.
 - [#8983](https://github.com/influxdata/influxdb/issues/8983): Remove the pidfile after the server has exited.
+- [#8986](https://github.com/influxdata/influxdb/issues/8986): Replace NewScanner to NewReader in importer.go file.


How about Add long-line support to client importer ?

e-dard · 2017-10-24T10:10:17Z

importer/v8/importer.go

-	if err := scanner.Err(); err != nil {
-		return fmt.Errorf("reading standard input: %s", err)
-	}
+	//if err := scanner.Err(); err != nil {


Please remove this commented out code.

e-dard · 2017-10-24T10:35:43Z

importer/v8/importer.go

-	for scanner.Scan() {
-		line := scanner.Text()
+func (i *Importer) processDDL(scanner *bufio.Reader) {
+	for {


There are a couple of issues here. (1) you could use ReadString(byte('\n')) instead of ReadLine(). That way you don't need to litter string(line) everywhere. Also, I think ReadLine() can return short reads, in that it might not return an entire line.

The second thing is you're not checking for errors that are not nil, but also not io.EOF. Those errors would now need to be returned to the caller too.

How about we go with something like this?

func (i *Importer) processDDL(scanner *bufio.Reader) error { for { line, err := scanner.ReadString(byte('\n')) if err != nil && err != io.EOF { return err } // If we find the DML token, we are done with DDL if strings.HasPrefix(line, "# DML") { return nil } if strings.HasPrefix(line, "#") { continue } // Skip blank lines if strings.TrimSpace(line) == "" { continue } i.queryExecutor(string(line)) // Check if this was the final token. if err == io.EOF { return nil } } return nil }

Don't forget you'll need to add error handling to the call to processDDL() inside of Import().

e-dard · 2017-10-24T10:36:08Z

importer/v8/importer.go

-		if strings.HasPrefix(line, "# CONTEXT-DATABASE:") {
-			i.database = strings.TrimSpace(strings.Split(line, ":")[1])
+	for {
+		line, _, err := scanner.ReadLine()


You should take a similar approach to the one I suggested previously in the review.

lets00 · 2017-10-24T13:21:33Z

I'm working on the proposed changes. Thank you for the feedback.

e-dard · 2017-10-24T13:23:41Z

I can confirm @lets00 has signed the CLA.

e-dard

Looking better but still a couple of style issues to fix.

e-dard · 2017-10-24T14:12:26Z

importer/v8/importer.go

@@ -112,7 +112,10 @@ func (i *Importer) Import() error {
 	scanner := bufio.NewReader(r)

 	// Process the DDL
-	i.processDDL(scanner)
+	ddl_err := i.processDDL(scanner)


if err := i.processDDL(scanner); err != nil {

e-dard · 2017-10-24T14:13:42Z

importer/v8/importer.go

-	i.processDDL(scanner)
+	ddl_err := i.processDDL(scanner)
+	if ddl_err != nil {
+		return fmt.Errorf("reading standard input: %s", ddl_err)


err not ddl_err

e-dard · 2017-10-24T14:14:00Z

importer/v8/importer.go

-	//if err := scanner.Err(); err != nil {
-	//	return fmt.Errorf("reading standard input: %s", err)
-	//}
+	dml_err := i.processDML(scanner)


if err := i.processDML(scanner); err != nil {

e-dard · 2017-10-24T14:14:07Z

importer/v8/importer.go

-	//}
+	dml_err := i.processDML(scanner)
+	if dml_err != nil {
+		return fmt.Errorf("reading standard input: %s", dml_err)


err not ddl_err

lets00 · 2017-10-24T14:15:51Z

importer/v8/importer.go

+		//End of File
+		if err == io.EOF {
+			return nil
+		}
 	}
 	// Call batchWrite one last time to flush anything out in the batch
 	i.batchWrite()


Maybe put i.batchWrite() on if err ==io.EOF statement

…OF statement

lets00 · 2017-10-26T14:01:42Z

I saw a infinite loop scenario when i.batchAccumulator() run after end of file. This not happen before(witch NewScanner), because the "for" statement not execute it. For fix this, I put io.EOF verification before i.batchAccumulator(). I will open a new pull request to fix this. Sorry about that.

e-dard

Hi @lets00

When testing this I found that the import will never complete properly. Specifically the influx process won't exit because the io.EOF check will never be triggered.

I'm you test this locally you should find the same—all the data will be imported, but the process will hang.

To fix this you need to move the:

//End of File?
		if err == io.EOF {
			// Call batchWrite one last time to flush anything out in the batch
			i.batchWrite()
			return nil
		}

blocks to be above the:

		// Skip blank lines
		if strings.TrimSpace(line) == "" {
			continue
		}

block in both the processDDL and processDML methods.

e-dard · 2017-10-26T15:19:52Z

@lets00 no need for a new pull request, just push a commit up to this one 😄

…loop

…fix-buffer-bug

lets00 · 2017-10-26T17:52:06Z

@e-dard I fix the problem putting if io.EOF... statement below the if err != nil ... statement in both the processDDL and processDML methods. I test in my environment, importing a monasca.dmp database and this solution works for me (not freeze like before).

e-dard

Almost there @lets00, just a couple of further style suggestions.

e-dard · 2017-10-27T10:04:21Z

importer/v8/importer.go

 		if strings.HasPrefix(line, "# CONTEXT-DATABASE:") {
-			i.database = strings.TrimSpace(strings.Split(line, ":")[1])
+			i.database = strings.TrimSpace(strings.Split(string(line), ":")[1])


You don't need string(line) since line is already a string.

e-dard · 2017-10-27T10:04:27Z

importer/v8/importer.go

 		}
 		if strings.HasPrefix(line, "# CONTEXT-RETENTION-POLICY:") {
-			i.retentionPolicy = strings.TrimSpace(strings.Split(line, ":")[1])
+			i.retentionPolicy = strings.TrimSpace(strings.Split(string(line), ":")[1])


You don't need string(line) since line is already a string.

e-dard · 2017-10-27T10:07:40Z

importer/v8/importer.go

+		line, err := scanner.ReadString(byte('\n'))
+		if err != nil && err != io.EOF {
+			return err
+		}


This can be an } else if err == io.EOF {.

e-dard · 2017-10-27T10:07:47Z

importer/v8/importer.go

+		line, err := scanner.ReadString(byte('\n'))
+		if err != nil && err != io.EOF {
+			return err
+		}


This can be an } else if err == io.EOF {.

…fix-buffer-bug

lets00 added 2 commits October 23, 2017 16:46

Change NewScanner to NewReader to fix bug influxdata#8986

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

03b5440

Update CHANGELOG.md

1d0e87a

ghost added the proposed label Oct 23, 2017

e-dard suggested changes Oct 24, 2017

View reviewed changes

Adding suggestions proposed by e-dard on pull request

12f1a88

e-dard suggested changes Oct 24, 2017

View reviewed changes

lets00 commented Oct 24, 2017

View reviewed changes

lets00 and others added 2 commits October 24, 2017 11:36

Some changes suggested by e-dard and move i.batchWrite() into if io.E…

01540df

…OF statement

Merge branch 'master' into fix-buffer-bug

ce17ebf

ghost assigned e-dard Oct 25, 2017

ghost added review and removed proposed labels Oct 25, 2017

e-dard approved these changes Oct 25, 2017

View reviewed changes

benbjohnson approved these changes Oct 25, 2017

View reviewed changes

e-dard suggested changes Oct 26, 2017

View reviewed changes

lets00 added 4 commits October 26, 2017 13:20

Change io.EOF statement before i.batchAccumulator() to avoid endless …

e1c6cdb

…loop

Remove extra space

3c2b918

Merge branch 'master' into fix-buffer-bug

ff9f817

Merge branch 'master' of https://github.com/influxdata/influxdb into …

09d1053

…fix-buffer-bug

e-dard suggested changes Oct 27, 2017

View reviewed changes

lets00 and others added 3 commits October 27, 2017 08:51

Merge branch 'master' of https://github.com/influxdata/influxdb into …

3cabdab

…fix-buffer-bug

Remove string(...) and chane if to else if statement

b3ea8b6

Update CHANGELOG.md

88b1b63

e-dard approved these changes Oct 30, 2017

View reviewed changes

e-dard merged commit 905945e into influxdata:master Oct 30, 2017

ghost removed the review label Oct 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace NewScanner to NewReader to fix bug #8986 #8999

Replace NewScanner to NewReader to fix bug #8986 #8999

lets00 commented Oct 23, 2017 •

edited

Loading

e-dard left a comment

e-dard Oct 24, 2017

e-dard Oct 24, 2017

e-dard Oct 24, 2017

e-dard Oct 24, 2017

e-dard Oct 24, 2017

lets00 commented Oct 24, 2017

e-dard commented Oct 24, 2017

e-dard left a comment

e-dard Oct 24, 2017

e-dard Oct 24, 2017

e-dard Oct 24, 2017

e-dard Oct 24, 2017

lets00 Oct 24, 2017 •

edited

Loading

lets00 commented Oct 26, 2017 •

edited

Loading

e-dard left a comment

e-dard commented Oct 26, 2017

lets00 commented Oct 26, 2017 •

edited

Loading

e-dard left a comment

e-dard Oct 27, 2017

e-dard Oct 27, 2017

e-dard Oct 27, 2017

e-dard Oct 27, 2017

Replace NewScanner to NewReader to fix bug #8986 #8999

Replace NewScanner to NewReader to fix bug #8986 #8999

Conversation

lets00 commented Oct 23, 2017 • edited Loading

e-dard left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lets00 commented Oct 24, 2017

e-dard commented Oct 24, 2017

e-dard left a comment

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

Choose a reason for hiding this comment

lets00 Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

lets00 commented Oct 26, 2017 • edited Loading

e-dard left a comment

Choose a reason for hiding this comment

e-dard commented Oct 26, 2017

lets00 commented Oct 26, 2017 • edited Loading

e-dard left a comment

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

Choose a reason for hiding this comment

lets00 commented Oct 23, 2017 •

edited

Loading

lets00 Oct 24, 2017 •

edited

Loading

lets00 commented Oct 26, 2017 •

edited

Loading

lets00 commented Oct 26, 2017 •

edited

Loading