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.

Sign up for GitHub

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

Jump to bottom

Add issue numbers to some unimplemented feature errors #42847

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Add issue numbers to some unimplemented feature errors #42847

merged 1 commit into from
Dec 3, 2019

Conversation

apantel
Copy link
Contributor

@apantel apantel commented Nov 27, 2019 •
edited
Loading

Partial fix for #42547
Release note (sql change): CockroachDB now provides a link to the
relevant github issue when clients attempt to use certain features that
are not yet implemented.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@apantel apantel changed the title Work in progress: Add issues to some unimplemented errors Add issue numbers to some unimplemented feature errors Dec 2, 2019
@knz
Copy link
Contributor

knz commented Dec 2, 2019

Thanks for this.

  1. please ensure the label X-anchored-telemetry is present on all the linked issues. This ensures that we don't mistakenly close the issue until we've checked that it's not referenced any more.

  2. this is a user-facing change (UX improvement) so you can create a summary of the improvement in a release note.

apantel
apantel commented Dec 2, 2019
View reviewed changes
Copy link
Contributor Author

@apantel apantel left a comment

Choose a reason for hiding this comment

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

I added the label to the issues that didn't have it. Thanks!

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

@knz
Copy link
Contributor

knz commented Dec 2, 2019 •
edited
Loading

👍

for the release note try this:

Release note (sql change): CockroachDB now provides a link to
the relevant github issue in the error message
when clients attempt to use certain features
that are not yet implemented.

(We don't yet have ux improvement as a category)

knz
knz suggested changes Dec 2, 2019
View reviewed changes
pkg/sql/virtual_schema.go Outdated
@@ -395,7 +395,7 @@ func (vs *VirtualSchemaHolder) getVirtualTableEntry(tn *tree.TableName) (virtual
return t, nil
}
if _, ok := db.allTableNames[tableName]; ok {
return virtualDefEntry{}, unimplemented.Newf(tn.Schema()+"."+tableName,
return virtualDefEntry{}, unimplemented.NewWithIssuef(8675,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are the rest okay?

pkg/sql/vars.go
@@ -156,7 +156,7 @@ var varGen = map[string]sessionVar{
case "utf8", "unicode", "cp65001":
return nil
default:
return unimplemented.Newf("client_encoding "+encoding,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait a minute can you please extend this to preserve the telemetry.

pkg/sql/sqlbase/structured.go Outdated
@@ -2634,11 +2634,11 @@ func (desc *MutableTableDescriptor) DropConstraint(

case ConstraintTypeFK:
if detail.FK.Validity == ConstraintValidity_Validating {
return unimplemented.Newf("drop-constraint-fk-mutation",
return unimplemented.NewWithIssuef(42844,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use WithTelemetry to differentiate the various FK operations so we get detailed usage telemetry.

@apantel
Add issue numbers to some unimplemented feature errors
ef177b2 
Partial fix for cockroachdb#42547

Release note (sql change): CockroachDB now provides a link to the
relevant github issue when clients attempt to use certain features that
are not yet implemented.
knz
knz approved these changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

top 👍

@awoods187
Copy link
Contributor

Very excited about this work!

@apantel
Copy link
Contributor Author

apantel commented Dec 3, 2019

bors r+

craig bot pushed a commit that referenced this pull request Dec 3, 2019
@apantel
Merge #42847
1d1cd78 
42847: Add issue numbers to some unimplemented feature errors r=apantel a=apantel

Partial fix for #42547
Release note (sql change): CockroachDB now provides a link to the
relevant github issue when clients attempt to use certain features that
are not yet implemented.

Co-authored-by: Adam Pantel <adam@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Dec 3, 2019

Build succeeded

@craig craig bot merged commit ef177b2 into cockroachdb:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@apantel @cockroach-teamcity @knz @awoods187