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

[NIT-2424] Improve error log when opening database #2353

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

gligneul
Copy link
Contributor

No description provided.

@gligneul gligneul self-assigned this May 29, 2024
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label May 29, 2024
@gligneul gligneul marked this pull request as ready for review May 30, 2024 01:14
Tristan-Wilson
Tristan-Wilson previously approved these changes May 30, 2024
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

LGTM

@ganeshvanahalli
Copy link
Contributor

The detailed error logging could be extended to other databases such as arbitrumdata and wasm along with l2chaindata, if an error occurs while opening them.

To simplify checks when there is an error, I think its good to have a wrapper function like

func WrapDBErrors(dbName string, dbErr error) error {
	// Similar to the logic you have currently to check if a db is misplaced in parent/grandparent dir
}

@gligneul
Copy link
Contributor Author

gligneul commented Jun 3, 2024

The detailed error logging could be extended to other databases such as arbitrumdata and wasm along with l2chaindata, if an error occurs while opening them.

To simplify checks when there is an error, I think its good to have a wrapper function like

I added a function called checkMisplacedDatabases to check all three databases.

cmd/nitro/init.go Outdated Show resolved Hide resolved
}

if !databaseEmpty(stack.InstanceDir()) {
return nil, nil, fmt.Errorf("found unexpected files in database directory '%s' (have you set --persistent.chain and --persistent.global-config correctly? If so, delete the database directory and try again)", stack.InstanceDir())
Copy link
Member

Choose a reason for hiding this comment

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

I would leave out the comment about persistent.chain and persistent.global-config. Most people wouldn't set it, and the people that do would most likely only set one of them. More likely that they mounted a directory incorrectly or had a bad tar file.

I think a better error message would be something like
fmt.Errorf("found %d unexpected files in database directory, including: %s", file_count, first_three_filenames)

@joshuacolvin0
Copy link
Member

The detailed error logging could be extended to other databases such as arbitrumdata and wasm along with l2chaindata, if an error occurs while opening them.

To simplify checks when there is an error, I think its good to have a wrapper function like

func WrapDBErrors(dbName string, dbErr error) error {
	// Similar to the logic you have currently to check if a db is misplaced in parent/grandparent dir
}

If databases are in different directories, it isn't really possible to give a comprehensive error. I think it is better to just look for a single database across a couple parent directories, otherwise just output simple error

@ganeshvanahalli
Copy link
Contributor

The detailed error logging could be extended to other databases such as arbitrumdata and wasm along with l2chaindata, if an error occurs while opening them.
To simplify checks when there is an error, I think its good to have a wrapper function like

func WrapDBErrors(dbName string, dbErr error) error {
	// Similar to the logic you have currently to check if a db is misplaced in parent/grandparent dir
}

If databases are in different directories, it isn't really possible to give a comprehensive error. I think it is better to just look for a single database across a couple parent directories, otherwise just output simple error

I agree, I meant to have a generalized function WrapDBErrors that can be used to add extra information when error occurs while opening a particular db, and not to check for each of them beforehand.
For example, in

nitro/cmd/nitro/init.go

Lines 295 to 298 in 2423bd8

chainData, err := stack.OpenDatabaseWithFreezerWithExtraOptions("l2chaindata", config.Execution.Caching.DatabaseCache, config.Persistent.Handles, config.Persistent.Ancient, "l2chaindata/", false, persistentConfig.Pebble.ExtraOptions("l2chaindata"))
if err != nil {
return nil, nil, err
}

instead of returning nil, nil, err, we could return nil, nil, WrapDBErrors("l2chaindata", err)

But if the check is only relevant to arbitrumdata, the generalization is not required.

@gligneul gligneul force-pushed the gligneul/improve-init-error-msg branch 2 times, most recently from a2a6f8e to 47e3b0c Compare June 4, 2024 14:13
- Add error message when database is misplaced
- Raise error if finds unexpected files
- Raise error when overwriting old database
- Prompt user to delete db when load fails
Copy link
Contributor

@magicxyyz magicxyyz left a comment

Choose a reason for hiding this comment

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

looks good, added one suggestion to the database dir misplacement checks

cmd/nitro/init.go Outdated Show resolved Hide resolved
magicxyyz
magicxyyz previously approved these changes Jun 6, 2024
Copy link
Contributor

@magicxyyz magicxyyz left a comment

Choose a reason for hiding this comment

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

LGTM

ganeshvanahalli
ganeshvanahalli previously approved these changes Jun 7, 2024
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli left a comment

Choose a reason for hiding this comment

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

LGTM

@gligneul gligneul merged commit 0e8ccd1 into master Jun 17, 2024
11 checks passed
@gligneul gligneul deleted the gligneul/improve-init-error-msg branch June 17, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants