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

importccl: Direct-ingest uses two bulk adders instead of one. #39424

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

adityamaru27
Copy link
Contributor

This is another change to stabilize direct ingest import before
it is made the default.
As a consequence of #39271, the number of files (L0 and total),
along with the cumulative compaction size increased drastically.
A consequence of no longer creating buckets of TableIDIndexID
before flushing is that the single bulk adder would receive a
mix of primary and secondary index entries. Since SSTs cannot
span across the splits we inserted between index spans, it would
create numerous, small secondary index SSTs along with the
bigger primary index SSTs, and flush on reaching its limit
(which would be often).

By introducing two adders, one for ingesting primary index data,
and the other for ingesting secondary index data we regain the
ability to make fewer, bigger secondary index SSTs and flush less
often. The peak mem is lower than what prebuffering used to
hit, while the number of files (L0 and total), and the cumulative
compaction size return to prebuffering levels.

Some stats below for a tpcc 1k, on a 1 node cluster.

With prebuffering:
Total Files : 7670
L0 Files : 1848
Cumulative Compaction (GB): 24.54GiB

Without prebuffering, one adder:
Total Files : 22420
L0 Files : 16900
Cumulative Compaction (GB): 52.43 GiB

Without prebuffering, two adders:
Total Files : 6805
L0 Files : 1078
Cumulative Compaction (GB): 18.89GiB

Release note: None

@adityamaru27 adityamaru27 requested a review from dt August 7, 2019 21:10
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

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


pkg/ccl/importccl/read_import_proc.go, line 390 at r1 (raw file):

				return err
			}
			pkIndexAdder.SetName("pkIndexAdder")

Not wild about the number of Set functions and mutable fields this thing is acquiring. I realize adding every single flag to the constructor is annoying given how that interface has to be maintained in sql, distsql and server, but I wonder if it is time to make a storagebase.BulkConfig{} struct we could pass to the constructor, and then just update that for every new flag.


pkg/ccl/importccl/read_import_proc.go, line 407 at r1 (raw file):

			pkAdded := pkIndexAdder.GetSummary()
			indexAdded := indexAdder.GetSummary()
			cumulativeAdded := &roachpb.BulkOpSummary{

there's a BulkOpSummary.Add(BulkOpSummary) method.

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

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


pkg/ccl/importccl/read_import_proc.go, line 390 at r1 (raw file):

Previously, dt (David Taylor) wrote…

Not wild about the number of Set functions and mutable fields this thing is acquiring. I realize adding every single flag to the constructor is annoying given how that interface has to be maintained in sql, distsql and server, but I wonder if it is time to make a storagebase.BulkConfig{} struct we could pass to the constructor, and then just update that for every new flag.

I forgot to mention that this does not need to block this PR -- just might be a nice followup.

This is another change to stabilize direct ingest import before
it is made the default.
As a consequence of cockroachdb#39271, the number of files (L0 and total),
along with the cumulative compaction size increased drastically.
A consequence of no longer creating buckets of TableIDIndexID
before flushing is that the single bulk adder would receive a
mix of primary and secondary index entries. Since SSTs cannot
span across the splits we inserted between index spans, it would
create numerous, small secondary index SSTs along with the
bigger primary index SSTs, and flush on reaching its limit
(which would be often).

By introducing two adders, one for ingesting primary index data,
and the other for ingesting secondary index data we regain the
ability to make fewer, bigger secondary index SSTs and flush less
often. The peak mem is lower than what prebuffering used to
hit, while the number of files (L0 and total), and the cumulative
compaction size return to prebuffering levels.

Some stats below for a tpcc 1k, on a 1 node cluster.

With prebuffering:
Total Files : 7670
L0 Files : 1848
Cumulative Compaction (GB): 24.54GiB

Without prebuffering, one adder:
Total Files : 22420
L0 Files : 16900
Cumulative Compaction (GB): 52.43 GiB

Without prebuffering, two adders:
Total Files : 6805
L0 Files : 1078
Cumulative Compaction (GB): 18.89GiB

Release note: None
Copy link
Contributor Author

@adityamaru27 adityamaru27 left a comment

Choose a reason for hiding this comment

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

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


pkg/ccl/importccl/read_import_proc.go, line 407 at r1 (raw file):

Previously, dt (David Taylor) wrote…

there's a BulkOpSummary.Add(BulkOpSummary) method.

Done.

Copy link
Contributor Author

@adityamaru27 adityamaru27 left a comment

Choose a reason for hiding this comment

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

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


pkg/ccl/importccl/read_import_proc.go, line 390 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I forgot to mention that this does not need to block this PR -- just might be a nice followup.

ack

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@adityamaru27
Copy link
Contributor Author

TFTR!
bors r+

craig bot pushed a commit that referenced this pull request Aug 8, 2019
39418: stats: truncate large datums when sampling for histogram r=rytaft a=rytaft

This commit adds logic to truncate long bit arrays, byte strings,
strings, and collated strings during sampling for histogram creation.
We do this to avoid using excessive memory or disk space during
sampling and storage of the final histogram.

Release note: None

39424: importccl: Direct-ingest uses two bulk adders instead of one. r=adityamaru27 a=adityamaru27

This is another change to stabilize direct ingest import before
it is made the default.
As a consequence of #39271, the number of files (L0 and total),
along with the cumulative compaction size increased drastically.
A consequence of no longer creating buckets of TableIDIndexID
before flushing is that the single bulk adder would receive a
mix of primary and secondary index entries. Since SSTs cannot
span across the splits we inserted between index spans, it would
create numerous, small secondary index SSTs along with the
bigger primary index SSTs, and flush on reaching its limit
(which would be often).

By introducing two adders, one for ingesting primary index data,
and the other for ingesting secondary index data we regain the
ability to make fewer, bigger secondary index SSTs and flush less
often. The peak mem is lower than what prebuffering used to
hit, while the number of files (L0 and total), and the cumulative
compaction size return to prebuffering levels.

Some stats below for a tpcc 1k, on a 1 node cluster.

With prebuffering:
Total Files : 7670
L0 Files : 1848
Cumulative Compaction (GB): 24.54GiB

Without prebuffering, one adder:
Total Files : 22420
L0 Files : 16900
Cumulative Compaction (GB): 52.43 GiB

Without prebuffering, two adders:
Total Files : 6805
L0 Files : 1078
Cumulative Compaction (GB): 18.89GiB

Release note: None

Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 8, 2019

Build succeeded

@craig craig bot merged commit e0d214f into cockroachdb:master Aug 8, 2019
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.

3 participants