Skip to content

Conversation

@dt
Copy link
Contributor

@dt dt commented Feb 27, 2019

This helper is intended to be called on the span(s) in which a caller
has been doing bulk-ingestion, to trigger consistency checks on the
range(s) in that span. This might be a good idea on its own but also has
the side effect of fixing up any MVCC stats inaccuracies that might
have been introduced by estimates during ingestion.

It is probably not critical to actually call this: eventually the
consistency queue will call anyway and fixup those stats if needed.
However, if we know we'll call it right away -- specifically before we
mark the operation that was ingesting as completed and turn those ranges
over to real traffic that might expect correct-ish stats -- we might be
able to make coarser, cheaper estimates, for instance during SST
ingestion where the current, accurate stats recomputation is expensive.

See #35231.

Release note: None

@dt dt requested review from a team, danhhz and nvb February 27, 2019 22:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt force-pushed the ingest-post-check branch from 2a95af5 to 01ab19b Compare February 27, 2019 22:59
@dt dt force-pushed the ingest-post-check branch from 01ab19b to c588ed2 Compare February 28, 2019 16:07
@dt dt requested a review from a team February 28, 2019 16:07
This helper is intended to be called on the span(s) in which a caller
has been doing bulk-ingestion, to trigger consistency checks on the
range(s) in that span. This might be a good idea on its own but also has
the side effect of fixing up any MVCC stats inaccuracies that might
have been introduced by estimates during ingestion.

It is probably not critical to actually call this: eventually the
consistency queue will call anyway and fixup those stats if needed.
However, if we know we'll call it right away -- specifically before we
mark the operation that was ingesting as completed and turn those ranges
over to real traffic that might expect correct-ish stats -- we might be
able to make coarser, cheaper estimates, for instance during SST
ingestion where the current, accurate stats recomputation is expensive.

See cockroachdb#35231.

Release note: None
@dt dt force-pushed the ingest-post-check branch from c588ed2 to c75fdef Compare February 28, 2019 16:38
@dt dt requested a review from a team as a code owner February 28, 2019 16:38
Copy link
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @dt)


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

	// If somehow we only ingested one key...
	if ingestSpan.Key.Equal(ingestSpan.EndKey) {

How will this happen? We should never have an ingestSpan where Key == EndKey.


pkg/ccl/importccl/sst_writer_proc.go, line 195 at r1 (raw file):

							log.Errorf(ctx, "failed to scatter span %s: %s", roachpb.PrettyPrintKey(nil, end), pErr)
						}
						if k, cur := sst.span.Key, ingestSpan.Key; cur == nil || k.Compare(cur) < 0 {

Here and above: can we use Span.Combine?


pkg/storage/storagebase/bulk_adder.go, line 50 at r1 (raw file):

		return nil
	}
	if ingested.Key.Equal(ingested.EndKey) {

Again, is this possible?

Copy link
Contributor Author

@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 @danhhz, @dt, and @nvanbenschoten)


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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How will this happen? We should never have an ingestSpan where Key == EndKey.

I thought I answered that in the comment on the check, but if the ingestion job ingested exactly one key (e.g. IMPORT of a single row)?

Copy link
Contributor

@nvb nvb 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 @danhhz and @dt)


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

Previously, dt (David Taylor) wrote…

I thought I answered that in the comment on the check, but if the ingestion job ingested exactly one key (e.g. IMPORT of a single row)?

Then shouldn't EndKey be either nil or Key.Next already? Span.EndKey should always be exclusive.

@dt
Copy link
Contributor Author

dt commented Apr 11, 2019

Closing this in favor of a core-driven change here, since it sounds like there're plans for how to actually clear ContainsEstimates which this doesn't do.

@dt dt closed this Apr 11, 2019
@dt dt deleted the ingest-post-check branch April 11, 2019 13:07
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