-
Notifications
You must be signed in to change notification settings - Fork 415
[DNM] Add debug log and test case #10678
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: JaySon-Huang <tshent@qq.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds observational logging across KVStore read/write and decoding flows, a new test exercising default-value decoding in RegionBlockReader, and minor include adjustments. No functional logic or public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@dbms/src/Storages/KVStore/Decode/RegionBlockReader.cpp`:
- Around line 47-50: The loop in RegionBlockReader::read currently logs every
data.value at INFO using Logger::get("dddddddddd") and unconditionally calls
toDebugString(); replace the ad-hoc logger with a cached LoggerPtr (e.g., log =
Logger::get("RegionBlockReader")) and change the logging to a TRACE/DEBUG level,
guarding the toDebugString() call so it is only executed when that level is
enabled (for example, if (log->trace()) { LOG_TRACE(log,
"RegionBlockReader::read data.value={}", data.value->toDebugString()); } ) to
avoid leaking data and excessive overhead.
In `@dbms/src/Storages/KVStore/MultiRaft/RaftCommands.cpp`:
- Line 371: The current INFO log in Region::handleWriteRaftCmd logs full KV
payloads using Logger::get("dddddddddd") and always calls
tikv_key.toDebugString()/tikv_value.toDebugString(); change this to use the
region's LoggerPtr (the region's `log`) and include region_id and CF/context in
the message, downgrade the level to TRACE (or DEBUG) and wrap the
toDebugString() calls in a log-level guard (e.g. if
log->isTraceEnabled()/isDebugEnabled()) to avoid the cost when the level is off;
ensure the message no longer uses the hardcoded "dddddddddd" logger and instead
references the region log with concise context instead of printing full payloads
at INFO.
In `@dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp`:
- Around line 696-744: The test ReadFromRegionDefaultValue currently only checks
that RegionBlockReader::read returns true and logs the decoded block—add
explicit assertions to validate the decoded result: use
createBlockSortByColumnID(decoding_schema) and inspect the block returned by
reader.read(*data_list_read, false) to assert the expected row count and that
specific columns (e.g., columns with ids for c2/c5/c4 as defined in TableInfo)
contain the correct default or origin_default values (and types) for each row;
remove the LOG_INFO call to avoid noisy output and replace it with
EXPECT_EQ/EXPECT_* checks on res_block.getColumnsWithTypeAndName() or helper
functions you already use to extract column contents (e.g.,
DB::tests::getColumnsContent) and assert exact strings/values. Ensure assertions
cover both presence of default values and their types/values per row so
regressions are detected.
In `@dbms/src/TiDB/Decode/RowCodec.cpp`:
- Around line 464-468: Replace the per-row LOG_INFO calls in RowV2 decoding (the
occurrences that use Logger::get("dddddddddd") and log
not_null_column_ids/null_column_ids) with a guarded lower‑level log (TRACE or
DEBUG) by first fetching a LoggerPtr with proper context (e.g., use the existing
class/module LoggerPtr pattern instead of Logger::get("dddddddddd")), check the
logger's level (logger->isTraceEnabled() or isDebugEnabled()) before
formatting/stringifying the vectors, and only then call LOG_TRACE/LOG_DEBUG with
the LoggerPtr and the prepared message; apply the same guarded change to the
other similar blocks referenced (the occurrences around the 510-521 and 598-607
regions) so per-row decoding avoids expensive unguarded formatting and ad‑hoc
logger names.
🧹 Nitpick comments (2)
dbms/src/Storages/KVStore/Decode/RegionBlockReader.cpp (1)
185-188: Downgrade schema snapshot dumps to TRACE/DEBUG and use a consistent logger.Dumping
column_definesat INFO on every read adds non‑trivial formatting overhead and clutters production logs. Consider TRACE/DEBUG with a cachedLoggerPtrinstead of"dddddddddd". As per coding guidelines, useLoggerPtrwithLOG_INFO/LOG_TRACEand relevant context.dbms/src/TiDB/Decode/RowCodec.cpp (1)
23-23: Use standard include style.Prefer angle‑bracket includes from
dbms/srcfor consistency (e.g.,<Common/FieldVisitors.h>). As per coding guidelines, use relative include paths with angle brackets.🔧 Suggested tweak
-#include "Common/FieldVisitors.h" +#include <Common/FieldVisitors.h>
|
Reproduce the tiflash and tikv inconsist problem with SQLs as the issue describe The trigger conditions are:
|
Signed-off-by: JaySon-Huang <tshent@qq.com>
|
Verify re-creating tiflash replica can fix the problem |
|
Summary: The encoded key-value pairs stored in the Raft log do not contain any field that perfectly indicates which schema version should be used for decoding a given key-value entry. To address this, TiFlash employs the following strategies to decode Raft log entries during ingestion:
However, under the current implementation, TiFlash cannot correctly handle a specific scenario: when a DDL alters a column from NOT NULL to NULLABLE, and concurrent Raft log entries containing NULL values for that column are written during the DDL execution window. In such cases, TiFlash erroneously fills in the original_default value instead of correctly interpreting the NULL. |
Signed-off-by: JaySon-Huang <tshent@qq.com>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
@JaySon-Huang: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.