test: investigate test failure due to wal log #22679
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new logging mechanism for PebbleDB in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
cool-develope,
kocubinski and
a team
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
store/v2/db/pebbledb.go (4)
33-33: Consider making log level configurableWhile suppressing info logs solves the immediate issue, having a configurable log level would provide more flexibility for debugging.
- Logger: &fatalLogger{}, // pebble info logs are messing up the logs (not a cosmossdk.io/log logger) + Logger: newPebbleLogger(opts), // Configure logger based on options
306-308: Document the fatalLogger typeAdd documentation explaining the purpose and behavior of this type.
+// fatalLogger implements pebble.Logger interface to control PebbleDB logging. +// It suppresses info logs while preserving fatal logs to prevent log pollution +// while ensuring critical errors are still reported. type fatalLogger struct { pebble.Logger }
310-312: Consider error handling for fatal logsThe current implementation directly forwards fatal logs to the default logger. Consider adding error handling or logging context.
func (*fatalLogger) Fatalf(format string, args ...interface{}) { + // Add store/v2/db context to the log message + format = "store/v2/db/pebble: " + format pebble.DefaultLogger.Fatalf(format, args...) }
314-314: Add documentation for Infof methodDocument why this method intentionally does nothing.
+// Infof implements pebble.Logger interface but suppresses info logs +// to prevent log pollution during normal operations. func (*fatalLogger) Infof(format string, args ...interface{}) {}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
store/v2/db/pebbledb.go(2 hunks)tests/systemtests/export_test.go(1 hunks)tests/systemtests/snapshots_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- tests/systemtests/snapshots_test.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/systemtests/export_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/db/pebbledb.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
tests/systemtests/export_test.go (1)
70-73: Consider a more robust solution for handling PebbleDB logs
While the current changes work around the issue by ignoring errors and capturing single output, this might mask potential issues. The comment indicates this is a temporary solution for test stability.
Let's verify if this is the only test affected by PebbleDB logs:
Consider implementing a more systematic approach to handle PebbleDB logs across all tests:
- Create a shared test utility for PebbleDB log management
- Implement proper log level control at the PebbleDB initialization
There was a problem hiding this comment.
The changes look good but the snapshot systemtest is still failing on CI
https://github.com/cosmos/cosmos-sdk/actions/runs/12401587454/job/34621294212?pr=22679#step:6:2209
julienrbrt
removed request for
kocubinski,
JulianToledano,
akhilkumarpilli,
cool-develope,
testinginprod and
hieuvubk
julienrbrt
unassigned kocubinski, JulianToledano, akhilkumarpilli, cool-develope, hieuvubk and testinginprod
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/snapshot/list.go (1)
27-29: Consider enhancing the "no snapshots" messageWhile the empty state handling is a good addition, the message could be more informative for users.
Consider this enhancement:
- cmd.Println("no snapshots found") + logger := client.GetLoggerFromCmd(cmd) + logger.Info("no snapshots found in the snapshot store")This change would:
- Use structured logging for consistency with other commands
- Provide more context about where we looked for snapshots
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/snapshot/list.go(1 hunks)server/v2/store/snapshot.go(3 hunks)tests/systemtests/snapshots_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/systemtests/snapshots_test.go
- server/v2/store/snapshot.go
🧰 Additional context used
📓 Path-based instructions (1)
client/snapshot/list.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
client/snapshot/list.go (1)
26-26: LGTM: Good use of empty lines for readability
The added empty lines improve code readability by clearly separating error handling, empty state handling, and iteration logic.
Also applies to: 30-30
(cherry picked from commit a9c1149) # Conflicts: # server/v2/api/telemetry/config.go # server/v2/stf/stf_test.go # server/v2/store/commands.go # server/v2/store/snapshot.go # server/v2/testdata/app.toml # store/v2/db/pebbledb.go # systemtests/CHANGELOG.md # systemtests/system.go # systemtests/testnet_init.go # tests/go.mod # tests/systemtests/go.mod # tests/systemtests/go.sum # x/accounts/go.mod # x/authz/go.mod # x/bank/go.mod # x/staking/go.mod
Co-authored-by: Julien Robert <[email protected]>
Description
#22577 took way longer than needed due to some logs added by the dbs.
It makes fail the systemtests on CI only (while passing locally).
This reverts the hack and investigate how to get a better fix (RunWithSingleOutput was added in #22577 but doesn't seem to work super well on CI still)
The more astonishing thing is that #22577 set the default db as goleveldb as well in simapp, so that the upgrade test keeps working, but those tests were still failing... while it is a pebbledb log line: https://github.com/cockroachdb/pebble/blob/21866e882237f27e9cfcc974b7b3031dd7543bda/open.go#L1031C9-L1031C15
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
localhost:7180.Bug Fixes
Tests