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

naming: delete old code that was needed for version compatibility #9516

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

deepthi
Copy link
Member

@deepthi deepthi commented Jan 15, 2022

Description

Delete code that was kept around until we passed v12.0
A few other miscellaneous fixes

Related Issue(s)

#7113

Checklist

  • Should this PR be backported? NO
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

… few other fixes

Signed-off-by: deepthi <deepthi@planetscale.com>
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

lgtm

// For backwards compatibility, we don't require tablet alias to be present in the response
// TODO(deepthi): After 7.0 change this
// return br.TransactionId, br.TabletAlias, nil
// also assert that br.TabletAlias == conn.tablet.Alias
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the scope of this TODO to also assert br.TabletAlias == conn.tablet.Alias.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that was a good idea at that time, but we don't in general put assertions into production code.
We could put an if condition that returns an error but it seems like overkill.

Copy link
Member Author

Choose a reason for hiding this comment

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

@systay @harshit-gangal any opinions on this?

Copy link
Member

Choose a reason for hiding this comment

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

I think this check is not necessary it is not giving any value here. as the conn here is the actual connection to the server it is unlikely it will differ.

// For backwards compatibility, we don't require tablet alias to be present in the response
// TODO(deepthi): After 7.0 change this
// return br.TransactionId, br.TabletAlias, nil
// also assert that br.TabletAlias == conn.tablet.Alias
Copy link
Member

Choose a reason for hiding this comment

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

I think this check is not necessary it is not giving any value here. as the conn here is the actual connection to the server it is unlikely it will differ.

@deepthi deepthi merged commit bacc8a4 into vitessio:main Jan 20, 2022
@deepthi deepthi deleted the ds-naming-fixes branch January 20, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: General Changes throughout the code base Type: Internal Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants