🛠️ Refactor suggestion

⚠️ Potential issue

Critical: Method requires implementation and documentation

The update_next_visible_time method is currently unimplemented but is crucial for the PR objective of processing change invisible time for orders. Several issues need to be addressed:

1. The method lacks documentation explaining its purpose and parameters
2. The implementation is missing despite having the necessary data structures (OrderInfo contains fields for invisible_time, offset_next_visible_time, and pop_time)

Consider adding documentation and implementing the method like this:

+ /// Updates the next visible time for a message in an ordered queue
+ /// 
+ /// # Parameters
+ /// * `topic` - The topic of the message
+ /// * `group` - The consumer group
+ /// * `queue_id` - The queue identifier
+ /// * `queue_offset` - The offset in the queue
+ /// * `pop_time` - The time when the message was popped
+ /// * `next_visible_time` - The next time the message should be visible
pub fn update_next_visible_time(
    &self,
    topic: &CheetahString,
    group: &CheetahString,
    queue_id: i32,
    queue_offset: u64,
    pop_time: u64,
    next_visible_time: u64,
) {
+    let mut wrapper = self.consumer_order_info_wrapper.lock();
+    let key = format!("{}@{}", topic, group);
+    
+    if let Some(queue_map) = wrapper.table.get_mut(&key) {
+        if let Some(order_info) = queue_map.get_mut(&queue_id) {
+            order_info.pop_time = pop_time;
+            order_info.offset_next_visible_time.insert(queue_offset, next_visible_time);
+        }
+    }
}

⚠️ Potential issue

Implement the queue_lock_manager method

The queue_lock_manager method is currently unimplemented. Since it's called elsewhere in the codebase, using this unimplemented method will result in a runtime panic. Please provide a proper implementation that returns a reference to a QueueLockManager instance to avoid potential errors.

⚠️ Potential issue

Avoid busy-wait loop when acquiring locks

The loop that repeatedly calls try_lock until it returns true can cause an asynchronous task to busy-wait, blocking the executor and leading to performance issues.

Consider refactoring to use an awaitable lock mechanism, such as tokio::sync::Mutex or tokio::sync::RwLock, which will asynchronously wait until the lock is available without blocking the executor.

⚠️ Potential issue

Ensure locks are released on all code paths

After acquiring the lock, the code checks if old_offset > request_header.offset and returns early if true. However, the lock is not released before this return statement, potentially leading to deadlocks as the lock remains held indefinitely.

Ensure that the lock is released before any early returns after acquiring it. You might use a guard that automatically releases the lock when it goes out of scope.

🛠️ Refactor suggestion

Use RAII guards to manage lock lifetimes

Manually managing the lock release increases the risk of forgetting to unlock in all code paths, especially when errors occur. Consider using RAII (Resource Acquisition Is Initialization) guards, such as tokio::sync::MutexGuard, which automatically release the lock when they go out of scope, ensuring that locks are always properly released.

⚠️ Potential issue

Ensure PopMessageProcessor is fully implemented before integration

The PopMessageProcessor is being initialized and integrated into BrokerRequestProcessor, but its methods, such as queue_lock_manager, are currently unimplemented. Integrating an unimplemented processor may lead to runtime panics when these methods are invoked.

Please complete the implementation of PopMessageProcessor before integrating it to prevent potential runtime errors.