importccl: Parallelize avro import #45269

miretskiy · 2020-02-21T00:15:09Z

Parallelize avro importer to improve its throughput (2.8x improvement).

Touches #40374.
Fixes #45097.

Release notes (performance): Faster avro import

cockroach-teamcity · 2020-02-21T00:15:20Z

This change is

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @spaskob)

pkg/ccl/importccl/read_import_avro.go, line 232 at r1 (raw file):

		return false
	}
	c.rejected <- fmt.Sprintf("%s\n", row)

I am confused, isn't avro a binary format? What is the meaning of row in this case?

pkg/ccl/importccl/read_import_base.go, line 356 at r1 (raw file):

type namedInput struct {
	reader *fileReader
	name   string

could you add comments what name and idx represent?

pkg/ccl/importccl/read_import_base.go, line 360 at r1 (raw file):

}

// A scanner over avro input.

why is the comment specific to avro?

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @spaskob)

pkg/ccl/importccl/read_import_avro.go, line 232 at r1 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

I am confused, isn't avro a binary format? What is the meaning of row in this case?

You are not confused; avro may or may not be binary.
However, at this stage, we have read avro input file and converted that input into a native go type
(for example: map, or an array, or a byte array, or an int, etc). So, i'm just printing that representation of the row that we have failed to handle.

I wonder if I should print %v instead though....

pkg/ccl/importccl/read_import_base.go, line 356 at r1 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

could you add comments what name and idx represent?

Done.

pkg/ccl/importccl/read_import_base.go, line 360 at r1 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

why is the comment specific to avro?

Because of copy & paste.

Updated.

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @spaskob)

pkg/ccl/importccl/read_import_base.go, line 360 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Because of copy & paste.

Updated.

Done.

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @spaskob)

pkg/ccl/importccl/read_import_avro.go, line 232 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

You are not confused; avro may or may not be binary.
However, at this stage, we have read avro input file and converted that input into a native go type
(for example: map, or an array, or a byte array, or an int, etc). So, i'm just printing that representation of the row that we have failed to handle.

I wonder if I should print %v instead though....

We may need to discuss how rejected file is going to be used. The initial idea is that it is in the same format as the input so that the user can repeat the import but only reading from the rejected file. If this is not feasible for avro we should reconsider this approach.

pkg/ccl/importccl/read_import_csv.go, line 82 at r2 (raw file):

}

func (c *csvInputReader) readFile(

I wonder if this function can be refactored away in the future as well.

pkg/ccl/importccl/read_import_csv.go, line 192 at r2 (raw file):

				col := conv.VisibleCols[i]
				err = wrapRowErr(err, s.input.name, rowNum, pgcode.Syntax,
					"parse %q as %s", col.Name, col.Type.SQLString())

I wonder if we should also include the field in the error message.

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @spaskob)

pkg/ccl/importccl/read_import_avro.go, line 232 at r1 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

We may need to discuss how rejected file is going to be used. The initial idea is that it is in the same format as the input so that the user can repeat the import but only reading from the rejected file. If this is not feasible for avro we should reconsider this approach.

It's not feasible for avro.

pkg/ccl/importccl/read_import_csv.go, line 82 at r2 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

I wonder if this function can be refactored away in the future as well.

perhaps... Not sure if it's possible to refactor e.g. mysqldump or pgdump formats,
but perhaps move this to parallelImporter in base and just call that

pkg/ccl/importccl/read_import_csv.go, line 192 at r2 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

I wonder if we should also include the field in the error message.

Perhaps -- I tried to keep things the same where possible. if you want, I can change this err message.

miretskiy · 2020-02-27T01:11:18Z

@spaskob @dt
finally: all ci builds are green. I found some issues (through existing tests), which I have fixed in avro records handling.

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @spaskob)

pkg/ccl/importccl/read_import_avro.go, line 232 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

It's not feasible for avro.

Done.

miretskiy · 2020-02-28T16:39:36Z

@dt @spaskob please review latest revision. Thanks.

dt

Reviewed 6 of 10 files at r1, 4 of 6 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @spaskob)

pkg/ccl/importccl/read_import_base.go, line 431 at r3 (raw file):

	Scan() bool

	// Err returns an error (if any) encountered when processing avro stream.

avro mentioned in comment

pkg/ccl/importccl/read_import_base.go, line 523 at r3 (raw file):

			// Scan more data, unless we're done.
			select {
			case <-ctx.Done():

I'm surprised this didn't show up in profiling, but .Done() in hot loops is usually performance problem -- we usually pull it into local above the loop done := ctx.Done() and even then sometimes even avoid checking the ch on every iteration as it is still somewhat expensive (i believe the prior CSV code only checked it on flushes).

pkg/ccl/importccl/read_import_base.go, line 568 at r3 (raw file):

	p.b.data = append(p.b.data, data)

	if len(p.b.data) == cap(p.b.data) {

this seems brittle, or at least subtle -- i had to read this a couple times before I'd convinced myself that this would actually behave correctly, i.e. that append or something else would never re-alloc the batch. IMO it'd be clearer just to add batchSize as a field on p.

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @spaskob)

pkg/ccl/importccl/read_import_base.go, line 431 at r3 (raw file):

Previously, dt (David Taylor) wrote…

avro mentioned in comment

Done.

pkg/ccl/importccl/read_import_base.go, line 523 at r3 (raw file):

Previously, dt (David Taylor) wrote…

I'm surprised this didn't show up in profiling, but .Done() in hot loops is usually performance problem -- we usually pull it into local above the loop done := ctx.Done() and even then sometimes even avoid checking the ch on every iteration as it is still somewhat expensive (i believe the prior CSV code only checked it on flushes).

Done.

pkg/ccl/importccl/read_import_base.go, line 568 at r3 (raw file):

Previously, dt (David Taylor) wrote…

this seems brittle, or at least subtle -- i had to read this a couple times before I'd convinced myself that this would actually behave correctly, i.e. that append or something else would never re-alloc the batch. IMO it'd be clearer just to add batchSize as a field on p.

Done.

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @spaskob)

pkg/ccl/importccl/read_import_csv.go, line 192 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Perhaps -- I tried to keep things the same where possible. if you want, I can change this err message.

You have 4 things to show but only 2 placeholders

Rerun and update stats for csv import benchmark. Release notes: None

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @spaskob)

pkg/ccl/importccl/read_import_base.go, line 523 at r3 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Done.

Good point. Removed, and, as per your suggestion moved to flush.

pkg/ccl/importccl/read_import_csv.go, line 192 at r2 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

You have 4 things to show but only 2 placeholders

You misread the code a bit; I've changed line wrapping to make it clear that i'm calling a function with 3 arguments (one of which is an error constructed with Wrapf(...))

miretskiy · 2020-03-02T19:08:12Z

I reran and updated before/after benchmark for csv import. Refactoring and introduction of the interface{} (instead of the string) didn't have a significant impact:
benchstat ~/before.txt ~/after.txt
name old time/op new time/op delta
ConvertRecord-16 2.27µs ±11% 2.29µs ± 8% ~ (p=0.494 n=10+10)

name old speed new speed delta
ConvertRecord-16 53.0MB/s ±12% 52.5MB/s ± 9% ~ (p=0.494 n=10+10)

name old alloc/op new alloc/op delta
ConvertRecord-16 3.63kB ± 0% 3.61kB ± 0% -0.68% (p=0.000 n=9+9)

name old allocs/op new allocs/op delta
ConvertRecord-16 100 ± 0% 101 ± 0% +1.00% (p=0.000 n=10+10)

Refactor csv importer and pull its parallel import related logic into separate helpers. This refactoring is done in preparation of making avro imports (and possibly others) use the same functionality. Release notes: None

Add a benchmark testing avro import performance. Release notes: None

Parallelize avro importer to improve its throughput (2.8x improvement). Fixes cockroachdb#45097 Release notes (performance): Faster avro import

miretskiy · 2020-03-02T19:57:19Z

bors r+

craig · 2020-03-02T21:08:35Z

Canceled (will resume)

miretskiy · 2020-03-02T21:12:04Z

bors r+

craig · 2020-03-02T21:12:06Z

Already running a review

craig · 2020-03-02T22:03:29Z

Build failed (retrying...)

GitHub CI (Cockroach)

miretskiy · 2020-03-02T23:13:51Z

bors r+

craig · 2020-03-02T23:13:53Z

Already running a review

craig · 2020-03-03T00:29:10Z

Build succeeded

GitHub CI (Cockroach)

miretskiy requested review from dt and spaskob February 21, 2020 00:15

miretskiy force-pushed the fast_avro branch 4 times, most recently from 6f93b6c to 4805318 Compare February 25, 2020 18:25

spaskob suggested changes Feb 26, 2020

View reviewed changes

miretskiy force-pushed the fast_avro branch 5 times, most recently from ab759ac to 5c30103 Compare February 26, 2020 17:42

miretskiy commented Feb 26, 2020

View reviewed changes

spaskob suggested changes Feb 26, 2020

View reviewed changes

miretskiy commented Feb 26, 2020

View reviewed changes

miretskiy force-pushed the fast_avro branch from 5c30103 to b49677e Compare February 26, 2020 22:36

miretskiy commented Feb 27, 2020

View reviewed changes

miretskiy force-pushed the fast_avro branch from b49677e to dc38d83 Compare February 28, 2020 16:35

miretskiy force-pushed the fast_avro branch 3 times, most recently from 746014a to fc53315 Compare February 28, 2020 23:20

dt reviewed Mar 2, 2020

View reviewed changes

miretskiy force-pushed the fast_avro branch 2 times, most recently from d407f37 to 6f9e247 Compare March 2, 2020 15:30

miretskiy commented Mar 2, 2020

View reviewed changes

miretskiy force-pushed the fast_avro branch from 6f9e247 to d55e1c8 Compare March 2, 2020 16:40

miretskiy force-pushed the fast_avro branch from d55e1c8 to 785681f Compare March 2, 2020 17:06

miretskiy requested review from spaskob and dt March 2, 2020 17:24

spaskob approved these changes Mar 2, 2020

View reviewed changes

importccl: Update csv processor benchmark.

dba963b

Rerun and update stats for csv import benchmark. Release notes: None

miretskiy commented Mar 2, 2020

View reviewed changes

Yevgeniy Miretskiy added 3 commits March 2, 2020 14:09

importccl: Refactor csv importer

0622c7f

Refactor csv importer and pull its parallel import related logic into separate helpers. This refactoring is done in preparation of making avro imports (and possibly others) use the same functionality. Release notes: None

importccl: Add avro import benchmark.

b815697

Add a benchmark testing avro import performance. Release notes: None

importccl: Parallelize avro import

7bd8e7d

Parallelize avro importer to improve its throughput (2.8x improvement). Fixes cockroachdb#45097 Release notes (performance): Faster avro import

miretskiy force-pushed the fast_avro branch from 785681f to 7bd8e7d Compare March 2, 2020 19:10

craig bot merged commit cf8cd92 into cockroachdb:master Mar 3, 2020

miretskiy deleted the fast_avro branch March 3, 2020 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importccl: Parallelize avro import #45269

importccl: Parallelize avro import #45269

miretskiy commented Feb 21, 2020 •

edited by spaskob

Loading

cockroach-teamcity commented Feb 21, 2020

spaskob left a comment

miretskiy left a comment

miretskiy left a comment

spaskob left a comment

miretskiy left a comment

miretskiy commented Feb 27, 2020

miretskiy left a comment

miretskiy commented Feb 28, 2020

dt left a comment

miretskiy left a comment

spaskob left a comment

miretskiy left a comment

miretskiy commented Mar 2, 2020

miretskiy commented Mar 2, 2020

craig bot commented Mar 2, 2020

miretskiy commented Mar 2, 2020

craig bot commented Mar 2, 2020

craig bot commented Mar 2, 2020

miretskiy commented Mar 2, 2020

craig bot commented Mar 2, 2020

craig bot commented Mar 3, 2020

importccl: Parallelize avro import #45269

importccl: Parallelize avro import #45269

Conversation

miretskiy commented Feb 21, 2020 • edited by spaskob Loading

cockroach-teamcity commented Feb 21, 2020

spaskob left a comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy commented Feb 27, 2020

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy commented Feb 28, 2020

dt left a comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy commented Mar 2, 2020

miretskiy commented Mar 2, 2020

craig bot commented Mar 2, 2020

Canceled (will resume)

miretskiy commented Mar 2, 2020

craig bot commented Mar 2, 2020

craig bot commented Mar 2, 2020

Build failed (retrying...)

miretskiy commented Mar 2, 2020

craig bot commented Mar 2, 2020

craig bot commented Mar 3, 2020

Build succeeded

miretskiy commented Feb 21, 2020 •

edited by spaskob

Loading