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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/are-you-alive/pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ func ParseDBName(connectionString string) string {
func ParseTabletType(connectionString string) string {
databaseName := ParseDBName(connectionString)
// for backwards compatibility
// TODO(deepthi): delete after v13.0
if strings.HasSuffix(databaseName, "@master") {
return "primary"
} else if strings.HasSuffix(databaseName, "@primary") {
Expand Down
22 changes: 11 additions & 11 deletions examples/compose/fix_replication.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@
# It handles the special case where the primary has purged bin logs that the replica requires.
# To use it place a mysql dump of the database on the same directory as this script.
# The name of the dump must be $KEYSPACE.sql. The script can also download the mysqldump for you.
# Replication is fixed by restoring the mysqldump and resetting the slave.
# Replication is fixed by restoring the mysqldump and resetting replication.
# https://dev.mysql.com/doc/refman/5.7/en/replication-mode-change-online-disable-gtids.html
# https://www.percona.com/blog/2013/02/08/how-to-createrestore-a-slave-using-gtid-replication-in-mysql-5-6/

cd "$(dirname "${BASH_SOURCE[0]}")"

function get_slave_status() {
# Get slave status
function get_replication_status() {
# Get replication status
STATUS_LINE=$(mysql -u$DB_USER -p$DB_PASS -h 127.0.0.1 -e "SHOW SLAVE STATUS\G")
LAST_ERRNO=$(grep "Last_IO_Errno:" <<< "$STATUS_LINE" | awk '{ print $2 }')
SLAVE_SQL_RUNNING=$(grep "Slave_SQL_Running:" <<< "$STATUS_LINE" | awk '{ print $2 }')
Expand All @@ -38,7 +38,7 @@ function get_slave_status() {
echo "Last_IO_Errno: $LAST_ERRNO"
}

function reset_slave() {
function reset_replication() {
# Necessary before sql file can be imported
echo "Importing MysqlDump: $KEYSPACE.sql"
mysql -u$DB_USER -p$DB_PASS -h 127.0.0.1 -e "RESET MASTER;STOP SLAVE;CHANGE MASTER TO MASTER_AUTO_POSITION = 0;source $KEYSPACE.sql;START SLAVE;"
Expand All @@ -47,20 +47,20 @@ function reset_slave() {
mysql -u$DB_USER -p$DB_PASS -h 127.0.0.1 -e "STOP SLAVE;CHANGE MASTER TO MASTER_AUTO_POSITION = 1;START SLAVE;"
}

# Retrieve slave status
get_slave_status
# Retrieve replication status
get_replication_status

# Exit script if called with argument 'status'
[ ${1:-''} != 'status' ] || exit 0;

# Check if SLAVE_IO is running
# Check if IO_Thread is running
if [[ $SLAVE_IO_RUNNING = "No" && $LAST_ERRNO = 1236 ]]; then

echo "Primary has purged bin logs that replica requires. Sync will require restore from mysqldump"
if [[ -f $KEYSPACE.sql ]] ; then
echo "mysqldump file $KEYSPACE.sql exists, attempting to restore.."
echo "Resetting slave.."
reset_slave
echo "Resetting replication.."
reset_replication
else
echo "Starting mysqldump. This may take a while.."
# Modify flags to user's requirements
Expand All @@ -69,8 +69,8 @@ if [[ $SLAVE_IO_RUNNING = "No" && $LAST_ERRNO = 1236 ]]; then
--no-autocommit --skip-comments --skip-add-drop-table --skip-add-locks \
--skip-disable-keys --single-transaction --set-gtid-purged=on --verbose > $KEYSPACE.sql; then
echo "mysqldump complete for database $KEYSPACE"
echo "Resetting slave.."
reset_slave
echo "Resetting replication.."
reset_replication
else
echo "mysqldump failed for database $KEYSPACE"
fi
Expand Down
5 changes: 1 addition & 4 deletions go/test/endtoend/cluster/cluster_process.go
Original file line number Diff line number Diff line change
Expand Up @@ -597,10 +597,7 @@ func (cluster *LocalProcessCluster) NewVtgateInstance() *VtgateProcess {
cluster.Cell,
cluster.Cell,
cluster.Hostname,
// version_upgrade test depends on using older binaries
// which means we cannot use the new PRIMARY tablet_type here
// TODO(deepthi): fix after v12.0
"MASTER,REPLICA",
"PRIMARY,REPLICA",
cluster.TopoProcess.Port,
cluster.TmpDirectory,
cluster.VtGateExtraArgs,
Expand Down
6 changes: 1 addition & 5 deletions go/vt/vttablet/grpctabletconn/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,7 @@ func (conn *gRPCQueryClient) Begin(ctx context.Context, target *querypb.Target,
if err != nil {
return 0, nil, tabletconn.ErrorFromGRPC(err)
}
// 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.

return br.TransactionId, conn.tablet.Alias, nil
return br.TransactionId, br.TabletAlias, nil
}

// Commit commits the ongoing transaction.
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vttablet/tabletconntest/fakequeryservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func (f *FakeQueryService) Begin(ctx context.Context, target *querypb.Target, op
if !proto.Equal(options, TestExecuteOptions) {
f.t.Errorf("invalid Execute.ExecuteOptions: got %v expected %v", options, TestExecuteOptions)
}
return beginTransactionID, nil, nil
return beginTransactionID, TestAlias, nil
}

// commitTransactionID is a test transaction id for Commit.
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vttablet/tabletserver/query_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ var sequenceFields = []*querypb.Field{

func (qre *QueryExecutor) shouldConsolidate() bool {
cm := qre.tsv.qe.consolidatorMode.Get()
return cm == tabletenv.Enable || ((cm == tabletenv.NotOnMaster || cm == tabletenv.NotOnPrimary) && qre.tabletType != topodatapb.TabletType_PRIMARY)
return cm == tabletenv.Enable || (cm == tabletenv.NotOnPrimary && qre.tabletType != topodatapb.TabletType_PRIMARY)
}

// Execute performs a non-streaming query execution.
Expand Down
8 changes: 3 additions & 5 deletions go/vt/vttablet/tabletserver/tabletenv/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,9 @@ import (

// These constants represent values for various config parameters.
const (
Enable = "enable"
Disable = "disable"
Dryrun = "dryRun"
// TODO(deepthi): Deprecated. Should be deleted after v12.0
NotOnMaster = "notOnMaster"
Enable = "enable"
Disable = "disable"
Dryrun = "dryRun"
NotOnPrimary = "notOnPrimary"
Polling = "polling"
Heartbeat = "heartbeat"
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vttablet/tabletserver/tabletserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -1949,7 +1949,7 @@ func (tsv *TabletServer) SetPassthroughDMLs(val bool) {
// SetConsolidatorMode sets the consolidator mode.
func (tsv *TabletServer) SetConsolidatorMode(mode string) {
switch mode {
case tabletenv.NotOnMaster, tabletenv.NotOnPrimary, tabletenv.Enable, tabletenv.Disable:
case tabletenv.NotOnPrimary, tabletenv.Enable, tabletenv.Disable:
tsv.qe.consolidatorMode.Set(mode)
}
}
Expand Down
2 changes: 0 additions & 2 deletions go/vt/worker/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,6 @@ func (e *executor) fetchWithRetries(ctx context.Context, action func(ctx context
if len(primaries) == 0 {
e.wr.Logger().Warningf("ExecuteFetch failed for keyspace/shard %v/%v because no PRIMARY is available; will retry until there is PRIMARY again", e.keyspace, e.shard)
statsRetryCount.Add(1)
// TODO(deepthi): for backwards compatibility. remove after v12.0
statsRetryCounters.Add(retryCategoryNoMasterAvailable, 1)
statsRetryCounters.Add(retryCategoryNoPrimaryAvailable, 1)
goto retry
}
Expand Down
8 changes: 3 additions & 5 deletions go/vt/worker/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,9 @@ var (
)

const (
retryCategoryReadOnly = "ReadOnly"
retryCategoryTimeoutError = "TimeoutError"
retryCategoryConnectionError = "ConnectionError"
// Deprecated. TODO(deepthi): remove after v12.0
retryCategoryNoMasterAvailable = "NoMasterAvailable"
retryCategoryReadOnly = "ReadOnly"
retryCategoryTimeoutError = "TimeoutError"
retryCategoryConnectionError = "ConnectionError"
retryCategoryNoPrimaryAvailable = "NoPrimaryAvailable"
)

Expand Down