testutils: add fingerprint utility package #104089
Conversation
|
This change is |
msbutler
force-pushed
the
butler-c2c-fingerprint-hook
branch
from
710eb87
to
8f98372
Compare
msbutler
force-pushed
the
butler-c2c-fingerprint-hook
branch
from
8f98372
to
5d33486
Compare
msbutler
requested review from
rharding6373,
herkolategan,
srosenberg,
rhu713 and
stevendanna
and removed request for
a team and
rharding6373
|
putting this on hold until #104219 merges |
msbutler
force-pushed
the
butler-c2c-fingerprint-hook
branch
4 times, most recently
from
2396ba5
to
148b0e2
Compare
This patch adds a couple helper methods that make it easier to: fingerprint a table, database or cluster, compare fingerprints, and identify a mismatch on the table level. All fingerprinting methods call crdb_internal.fingerprint() on the table span level with the user provided options. This patch also integrates the new methods in the backupccl and streamingccl codebases. A future patch could add a helper method that identifies mismatched keys within a mismatched table. Informs cockroachdb#103072 Release note: None
msbutler
force-pushed
the
butler-c2c-fingerprint-hook
branch
from
148b0e2
to
bad4864
Compare
|
unrelated flake in extended CI. @rhu713 @stevendanna Please take a look! |
| func CompareDatabaseFingerprints(db1, db2 map[string]int64) error { | ||
| for tableName, tableFingerprint := range db1 { | ||
| table2Fingerprint, ok := db2[tableName] | ||
| if !ok { | ||
| return errors.Newf("%s table not in second database", tableName) | ||
| } | ||
| if tableFingerprint != table2Fingerprint { | ||
| return errors.Newf("fingerprint mismatch on %s table: %d != %d", tableName, | ||
| tableFingerprint, table2Fingerprint) | ||
| } | ||
| delete(db2, tableName) | ||
| } | ||
| if len(db2) > 0 { | ||
| return errors.Newf("second database has more tables %s", db2) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func CompareClusterFingerprints(c1, c2 map[string]map[string]int64) error { | ||
| for dbName, dbFingerprints := range c1 { | ||
| db2Fingerprints, ok := c2[dbName] | ||
| if !ok { | ||
| return errors.Newf("%s table not in second database", dbName) | ||
| } | ||
| if err := CompareDatabaseFingerprints(dbFingerprints, db2Fingerprints); err != nil { | ||
| return fmt.Errorf("failed comparing fingerprints for database %s: %w", dbName, err) | ||
| } | ||
| delete(c2, dbName) | ||
| } | ||
| if len(c2) > 0 { | ||
| return errors.Newf("second cluster has more databases %s", c2) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fine to keep but I wonder how much clearer this output is than require.Equal on the two input maps.
There was a problem hiding this comment.
I kept away from the require package in this pr because I think the caller should decide how to handle any error this package bubbles up. require tends to fatal anything it doesn't like.
There was a problem hiding this comment.
Sure, I more meant whether we need this custom comparison function at all.
There was a problem hiding this comment.
Ah i see. I like this func because it will point you to exactly what's wrong with your fingerprint comparison. require over the maps implies that the test debugger has to squint at printed maps :D.
I could also see a future where we scan over the mismatched table to find the wrong key.
There was a problem hiding this comment.
@stevendanna thanks for the review. Ready for another look!
| func CompareDatabaseFingerprints(db1, db2 map[string]int64) error { | ||
| for tableName, tableFingerprint := range db1 { | ||
| table2Fingerprint, ok := db2[tableName] | ||
| if !ok { | ||
| return errors.Newf("%s table not in second database", tableName) | ||
| } | ||
| if tableFingerprint != table2Fingerprint { | ||
| return errors.Newf("fingerprint mismatch on %s table: %d != %d", tableName, | ||
| tableFingerprint, table2Fingerprint) | ||
| } | ||
| delete(db2, tableName) | ||
| } | ||
| if len(db2) > 0 { | ||
| return errors.Newf("second database has more tables %s", db2) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func CompareClusterFingerprints(c1, c2 map[string]map[string]int64) error { | ||
| for dbName, dbFingerprints := range c1 { | ||
| db2Fingerprints, ok := c2[dbName] | ||
| if !ok { | ||
| return errors.Newf("%s table not in second database", dbName) | ||
| } | ||
| if err := CompareDatabaseFingerprints(dbFingerprints, db2Fingerprints); err != nil { | ||
| return fmt.Errorf("failed comparing fingerprints for database %s: %w", dbName, err) | ||
| } | ||
| delete(c2, dbName) | ||
| } | ||
| if len(c2) > 0 { | ||
| return errors.Newf("second cluster has more databases %s", c2) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I kept away from the require package in this pr because I think the caller should decide how to handle any error this package bubbles up. require tends to fatal anything it doesn't like.
|
unrelated extended CI flake. |
|
TFTR! bors r=stevendanna |
|
Build failed (retrying...): |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers, but backport branch blathers/backport-release-23.1-104089 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/104918/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This patch adds a couple helper methods that make it easier to: fingerprint a table, database or cluster, compare fingerprints, and identify a mismatch on the table level. All fingerprinting methods call crdb_internal.fingerprint() on the table span level with the user provided options.
This patch also integrates the new methods in the backupccl and streamingccl codebases.
A future patch could add a helper method that identifies mismatched keys within a mismatched table.
Informs #103072
Release note: None