Skip to content

Conversation

FGasper
Copy link
Collaborator

@FGasper FGasper commented Sep 9, 2025

This makes Migration Verifier enqueue rechecks to a local BadgerDB datastore rather than the metadata DB. This avoids problems with latency and allows the metadata to reside on the destination cluster without materially impeding handling of change events. The local DB is removed whenever the --clean flag is given.

Since it is now sensible to store metadata on the destination cluster, this changeset also makes the --metaURI parameter default to the --dstURI parameter’s value.

It may be sensible in the future to migrate all of metadata to BadgerDB.

@FGasper FGasper marked this pull request as ready for review September 9, 2025 20:42
@FGasper FGasper requested a review from tdq45gj September 9, 2025 20:43

const (
schemaVersionKey = "formatVersion"
schemaVersion = uint16(3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is schema version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar to the comparable metaDB mechanism. How we prevent inadvertent breakage from using an outdated localDB across verifier updates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds like each time we change the schema we should update the schema version. Is it documented or tested anywhere?

if errors.Is(err, badger.ErrKeyNotFound) {
return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we not handling other errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

key-not-found is expected when the DB is new. There should be no other errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add an assertion that no other error should occur?

@FGasper FGasper requested a review from tdq45gj September 10, 2025 23:51
Copy link
Collaborator

@tdq45gj tdq45gj left a comment

Choose a reason for hiding this comment

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

LGTM

return errors.Wrapf(
ldb.db.Update(func(tx *badger.Txn) error {
var addedRechecks uint64
for _, chunk := range lo.Chunk(rechecks, 16384) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

chunk size could be a const.

Copy link
Collaborator

@tdq45gj tdq45gj left a comment

Choose a reason for hiding this comment

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

LGTM

@FGasper
Copy link
Collaborator Author

FGasper commented Sep 15, 2025

This doesn’t actually work very well. It’s more performant to spin up a local MongoDB replset.

@FGasper FGasper closed this Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants