stats,distsqlrun: improve create stats job progress reporting #35684
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, @RaduBerinde, and @rytaft)
pkg/sql/distsqlpb/processors.proto, line 820 at r1 (raw file):
// The total number of rows expected in the table based on previous runs of // CREATE STATISTICS. Used for progress reporting.
[nit] mention that if it is 0, the reported progress is 0 until the very end.
pkg/sql/distsqlrun/sampler.go, line 197 at r1 (raw file):
// report number of rows processed since the last update. meta := &ProducerMetadata{ Progress: &jobspb.Progress{Details: &jobspb.Progress_CreateStats{
I am not crazy about ProducerMetadata containing a jobspb.Progress message. The Progress message supports a wide array of things and we're only using it for a very specific case. We are also abusing the semantics because CreateStatsProgress.RowsProcessed is the number of rows processed so far but here we are using it as number of rows processed since the last update.
Can we replace ProducerMetadata.Progress with a more specific SamplerRowsProcessed?
And if we do that, do we still need CreateStatsProgress.RowsProcessed? That struct is meant to be updated in FractionProgress to be stored in the jobs table but we aren't doing that AFAICT (and we don't need it).
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlpb/processors.proto, line 820 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] mention that if it is 0, the reported progress is 0 until the very end.
Done.
pkg/sql/distsqlrun/sampler.go, line 197 at r1 (raw file):
Previously, RaduBerinde wrote…
I am not crazy about ProducerMetadata containing a
jobspb.Progressmessage. The Progress message supports a wide array of things and we're only using it for a very specific case. We are also abusing the semantics becauseCreateStatsProgress.RowsProcessedis the number of rows processed so far but here we are using it as number of rows processed since the last update.Can we replace ProducerMetadata.Progress with a more specific
SamplerRowsProcessed?And if we do that, do we still need
CreateStatsProgress.RowsProcessed? That struct is meant to be updated inFractionProgressto be stored in the jobs table but we aren't doing that AFAICT (and we don't need it).
Makes sense. I created a new ProducerMetadata message called SamplerProgress. (I wanted to make it a bit more general in case we add other types of progress such as ranges processed).
rytaft
force-pushed
the
progress
branch
3 times, most recently
from
ddc949f
to
5fa3899
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlpb/data.proto, line 273 at r2 (raw file):
roachpb.TxnCoordMeta txn_coord_meta = 4; RowNum row_num = 5; SamplerProgress sampler_progress = 7;
[nit] reserve 6?
There was a problem hiding this comment.
[nit] probably more of an "admin ui change" than "sql change"
Reviewable status:
rytaft
force-pushed
the
progress
branch
2 times, most recently
from
064a490
to
ea6a4a9
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlpb/data.proto, line 273 at r2 (raw file):
Previously, RaduBerinde wrote…
[nit] reserve 6?
Done. (reserved doesn't work inside oneof, but it seems to have the same effect on the outside...)
Prior to this commit, all create stats jobs only reported progress when each node had completed all processing. This resulted in a poor quality user experience, since long create stats jobs would appear to have made zero progress for a long period of time. This commit adds logic to report more accurate incremental progress by counting the number of rows processed so far and dividing that by the total number of rows in the table (estimated according to the latest stats). The first job for a given table reports 0 progress until completion (since there are no stats available yet), but all subsequent jobs will now have better progress estimation. Fixes cockroachdb#35360 Informs cockroachdb#35346 Release note (admin ui change): Improved progress reporting for CREATE STATISTICS jobs.
There was a problem hiding this comment.
bors r+
Reviewable status:
35684: stats,distsqlrun: improve create stats job progress reporting r=rytaft a=rytaft Prior to this commit, all create stats jobs only reported progress when each node had completed all processing. This resulted in a poor quality user experience, since long create stats jobs would appear to have made zero progress for a long period of time. This commit adds logic to report more accurate incremental progress by counting the number of rows processed so far and dividing that by the total number of rows in the table (estimated according to the latest stats). The first job for a given table reports 0 progress until completion (since there are no stats available yet), but all subsequent jobs will now have better progress estimation. Fixes #35360 Informs #35346 Release note (admin ui change): Improved progress reporting for CREATE STATISTICS jobs. 35687: sql/parser: rework the error reporting mechanism r=knz a=knz Needed for https://github.com/cockroachlabs/registration/issues/203 Prior to this patch, the parser error handling code was unnecessary complex, and had two major shortcomings: - any detailed error object was "flattened" to a simple string when converted into a syntax error, resulting in loss of information. This was regrettable as any unimplemented error produced "underneath" the parser (say, in `coltypes.ArrayOf()`) would lose the link to the related issue during that process. - users would not benefit from the informational and educatinal hint available in errors generated by pgerror.Unimplemented(). This patch fixes this. Release note: None Co-authored-by: Rebecca Taft <becca@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
Prior to this commit, all create stats jobs only reported progress when
each node had completed all processing. This resulted in a poor quality user
experience, since long create stats jobs would appear to have made zero
progress for a long period of time. This commit adds logic to report more
accurate incremental progress by counting the number of rows processed so
far and dividing that by the total number of rows in the table (estimated
according to the latest stats). The first job for a given table reports
0 progress until completion (since there are no stats available yet), but
all subsequent jobs will now have better progress estimation.
Fixes #35360
Informs #35346
Release note (admin ui change): Improved progress reporting for CREATE
STATISTICS jobs.