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

Reintroduce TestReadOutdatedInstanceKeys with debugging information #13562

Merged
merged 1 commit into from
Jul 26, 2023
Merged

Reintroduce TestReadOutdatedInstanceKeys with debugging information #13562

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 17 additions & 4 deletions go/vt/vtorc/inst/instance_dao_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/vt/external/golib/sqlutils"
"vitess.io/vitess/go/vt/log"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
"vitess.io/vitess/go/vt/topo/topoproto"
"vitess.io/vitess/go/vt/vtorc/config"
Expand Down Expand Up @@ -381,7 +382,6 @@ func TestReadInstancesByCondition(t *testing.T) {

// TestReadOutdatedInstanceKeys is used to test the functionality of ReadOutdatedInstanceKeys and verify its failure modes and successes.
func TestReadOutdatedInstanceKeys(t *testing.T) {
t.Skipf("Skipping the test until flakiness can be resolved.")
// The test is intended to be used as follows. The initial data is stored into the database. Following this, some specific queries are run that each individual test specifies to get the desired state.
tests := []struct {
name string
Expand All @@ -396,7 +396,7 @@ func TestReadOutdatedInstanceKeys(t *testing.T) {
name: "One instance is outdated",
sql: []string{
"update database_instance set last_checked = now()",
"update database_instance set last_checked = time(now(), '-1 hour') where alias = 'zone1-0000000100'",
"update database_instance set last_checked = datetime(now(), '-1 hour') where alias = 'zone1-0000000100'",
},
instancesRequired: []string{"zone1-0000000100"},
}, {
Expand All @@ -410,7 +410,7 @@ func TestReadOutdatedInstanceKeys(t *testing.T) {
name: "One instance doesn't have myql data and one is outdated",
sql: []string{
"update database_instance set last_checked = now()",
"update database_instance set last_checked = time(now(), '-1 hour') where alias = 'zone1-0000000100'",
"update database_instance set last_checked = datetime(now(), '-1 hour') where alias = 'zone1-0000000100'",
`INSERT INTO vitess_tablet VALUES('zone1-0000000103','localhost',7706,'ks','0','zone1',2,'0001-01-01 00:00:00+00:00','');`,
},
instancesRequired: []string{"zone1-0000000103", "zone1-0000000100"},
Expand All @@ -427,7 +427,7 @@ func TestReadOutdatedInstanceKeys(t *testing.T) {
forgetAliases = oldCache
config.Config.InstancePollSeconds = oldVal
}()
config.Config.InstancePollSeconds = 60 * 59
config.Config.InstancePollSeconds = 60 * 25
forgetAliases = cache.New(time.Minute, time.Minute)

for _, tt := range tests {
Expand All @@ -443,6 +443,19 @@ func TestReadOutdatedInstanceKeys(t *testing.T) {
}

tabletAliases, err := ReadOutdatedInstanceKeys()

errInDataCollection := db.QueryVTOrcRowsMap(`select alias,
last_checked,
last_attempted_check,
ROUND((JULIANDAY(now()) - JULIANDAY(last_checked)) * 86400) AS difference,
last_attempted_check <= last_checked as use1,
last_checked < now() - interval 1500 second as is_outdated1,
last_checked < now() - interval 3000 second as is_outdated2
from database_instance`, func(rowMap sqlutils.RowMap) error {
log.Errorf("Row in database_instance - %+v", rowMap)
return nil
})
require.NoError(t, errInDataCollection)
require.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should do that check after line 445

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so. I want to collect the data irrespective of us getting an error or not. Basically after we ahve run the function, I want to know the database state, before any checks fail. Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it makes sense, as long as this was not something you missed when creating the PR i am fine with it.

require.ElementsMatch(t, tabletAliases, tt.instancesRequired)
})
Expand Down