github-actions[bot] commented on code in PR #64798:
URL: https://github.com/apache/doris/pull/64798#discussion_r3474528438
##########
be/src/io/cache/lru_queue_recorder.cpp:
##########
@@ -58,54 +58,62 @@ size_t LRUQueueRecorder::replay_queue_event(FileCacheType
type) {
CacheLRULogQueue& log_queue = get_lru_log_queue(type);
LRUQueue& shadow_queue = get_shadow_queue(type);
- std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
- std::unique_ptr<CacheLRULog> log;
+ size_t idx = file_cache_type_index(type);
size_t replayed = 0;
- while (log_queue.try_dequeue(log)) {
- release_lru_log_queue_slot(type);
- ++replayed;
- try {
- switch (log->type) {
- case CacheLRULogType::ADD: {
- shadow_queue.add(log->hash, log->offset, log->size,
lru_log_lock);
- break;
- }
- case CacheLRULogType::REMOVE: {
- auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
- if (it != std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
- shadow_queue.remove(it, lru_log_lock);
- } else {
- LOG(WARNING) << "REMOVE failed, doesn't exist in shadow
queue";
+ {
+ std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
+ std::unique_ptr<CacheLRULog> log;
+ while (log_queue.try_dequeue(log)) {
+ release_lru_log_queue_slot(type);
+ ++replayed;
+ try {
+ switch (log->type) {
+ case CacheLRULogType::ADD: {
+ shadow_queue.add(log->hash, log->offset, log->size,
lru_log_lock);
+ limit_shadow_queue_size(shadow_queue, lru_log_lock);
Review Comment:
This cap changes the dumped queue from the cold prefix that the restart path
effectively relied on into the hot suffix. On restart, `initialize_unlocked()`
restores the dump before `_storage->init()` loads the full metadata set;
`restore_queue()` appends the dumped entries first, then the metadata loader
skips those entries and appends every missing block via `add_cell()`. With
offsets 0,1,2,3,4 and a limit of 2, the shadow queue dumped here is 3,4, so
restart rebuilds 3,4 first and then appends 0,1,2, making the cold blocks newer
than the dumped hot tail. Please apply the dumped ordering after metadata load
or persist enough ordering information to reconstruct the final queue order.
##########
be/src/io/cache/lru_queue_recorder.cpp:
##########
@@ -58,54 +58,62 @@ size_t LRUQueueRecorder::replay_queue_event(FileCacheType
type) {
CacheLRULogQueue& log_queue = get_lru_log_queue(type);
LRUQueue& shadow_queue = get_shadow_queue(type);
- std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
- std::unique_ptr<CacheLRULog> log;
+ size_t idx = file_cache_type_index(type);
size_t replayed = 0;
- while (log_queue.try_dequeue(log)) {
- release_lru_log_queue_slot(type);
- ++replayed;
- try {
- switch (log->type) {
- case CacheLRULogType::ADD: {
- shadow_queue.add(log->hash, log->offset, log->size,
lru_log_lock);
- break;
- }
- case CacheLRULogType::REMOVE: {
- auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
- if (it != std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
- shadow_queue.remove(it, lru_log_lock);
- } else {
- LOG(WARNING) << "REMOVE failed, doesn't exist in shadow
queue";
+ {
+ std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
+ std::unique_ptr<CacheLRULog> log;
+ while (log_queue.try_dequeue(log)) {
+ release_lru_log_queue_slot(type);
+ ++replayed;
+ try {
+ switch (log->type) {
+ case CacheLRULogType::ADD: {
+ shadow_queue.add(log->hash, log->offset, log->size,
lru_log_lock);
+ limit_shadow_queue_size(shadow_queue, lru_log_lock);
+ break;
}
- break;
- }
- case CacheLRULogType::MOVETOBACK: {
- auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
- if (it != std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
- shadow_queue.move_to_end(it, lru_log_lock);
- } else {
- LOG(WARNING) << "MOVETOBACK failed, doesn't exist in
shadow queue";
+ case CacheLRULogType::REMOVE: {
+ auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
+ if (it !=
std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
+ shadow_queue.remove(it, lru_log_lock);
+ } else {
+ LOG(WARNING) << "REMOVE failed, doesn't exist in
shadow queue";
+ }
+ limit_shadow_queue_size(shadow_queue, lru_log_lock);
Review Comment:
`limit_shadow_queue_size()` can only drop extra entries; it cannot backfill
the bounded tail after a remove. With a limit of 3, ADD replay for offsets
0,1,2,3,4 leaves the shadow as 2,3,4. If offset 3 is removed, the real queue
tail should become 1,2,4, but this branch removes 3 and leaves only 2,4 because
entry 1 was already discarded. The next dump then has fewer/order-incomplete
retained entries. Please keep enough predecessor state, rebuild/apply ordering
after loading full metadata, or otherwise avoid trimming in a way that prevents
REMOVE from preserving the configured tail.
##########
be/src/io/cache/lru_queue_recorder.cpp:
##########
@@ -58,54 +58,62 @@ size_t LRUQueueRecorder::replay_queue_event(FileCacheType
type) {
CacheLRULogQueue& log_queue = get_lru_log_queue(type);
LRUQueue& shadow_queue = get_shadow_queue(type);
- std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
- std::unique_ptr<CacheLRULog> log;
+ size_t idx = file_cache_type_index(type);
size_t replayed = 0;
- while (log_queue.try_dequeue(log)) {
- release_lru_log_queue_slot(type);
- ++replayed;
- try {
- switch (log->type) {
- case CacheLRULogType::ADD: {
- shadow_queue.add(log->hash, log->offset, log->size,
lru_log_lock);
- break;
- }
- case CacheLRULogType::REMOVE: {
- auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
- if (it != std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
- shadow_queue.remove(it, lru_log_lock);
- } else {
- LOG(WARNING) << "REMOVE failed, doesn't exist in shadow
queue";
+ {
+ std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
+ std::unique_ptr<CacheLRULog> log;
+ while (log_queue.try_dequeue(log)) {
+ release_lru_log_queue_slot(type);
+ ++replayed;
+ try {
+ switch (log->type) {
+ case CacheLRULogType::ADD: {
+ shadow_queue.add(log->hash, log->offset, log->size,
lru_log_lock);
+ limit_shadow_queue_size(shadow_queue, lru_log_lock);
+ break;
}
- break;
- }
- case CacheLRULogType::MOVETOBACK: {
- auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
- if (it != std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
- shadow_queue.move_to_end(it, lru_log_lock);
- } else {
- LOG(WARNING) << "MOVETOBACK failed, doesn't exist in
shadow queue";
+ case CacheLRULogType::REMOVE: {
+ auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
+ if (it !=
std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
+ shadow_queue.remove(it, lru_log_lock);
+ } else {
+ LOG(WARNING) << "REMOVE failed, doesn't exist in
shadow queue";
+ }
+ limit_shadow_queue_size(shadow_queue, lru_log_lock);
+ break;
}
- break;
- }
- case CacheLRULogType::RESIZE: {
- auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
- if (it != std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
- shadow_queue.resize(it, log->size, lru_log_lock);
- } else {
- LOG(WARNING) << "RESIZE failed, doesn't exist in shadow
queue";
+ case CacheLRULogType::MOVETOBACK: {
+ auto it = shadow_queue.get(log->hash, log->offset,
lru_log_lock);
+ if (it !=
std::list<LRUQueue::FileKeyAndOffset>::iterator()) {
+ shadow_queue.move_to_end(it, lru_log_lock);
+ } else {
+ LOG(WARNING) << "MOVETOBACK failed, doesn't exist in
shadow queue";
Review Comment:
After capping the shadow queue, this branch also needs to handle touches for
entries that were trimmed out. For example, with a limit of 3, replaying ADDs
for offsets 0,1,2,3,4 leaves the shadow queue as 2,3,4. If offset 0 is read,
`use_cell()`/`update_block_lru()` records a `MOVETOBACK` and the real queue
tail becomes 3,4,0, but this branch only logs when 0 is missing from the capped
shadow queue and leaves 2,3,4 to be dumped/restored. That loses the recently
touched block from the restored LRU tail. Please treat a missing `MOVETOBACK`
as adding `(hash, offset, size)` before enforcing the cap, and enforce the cap
after `MOVETOBACK`/`RESIZE` as well so lowered mutable limits are honored.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
