-
Notifications
You must be signed in to change notification settings - Fork 112
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
[ISSUE #696]🚀Support pull message consume-5 #697
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -366,7 +366,7 @@ impl MappedFile for DefaultMappedFile { | |
start_offset: self.file_from_offset + pos as u64, | ||
size, | ||
mapped_file: Some(self), | ||
is_in_cache: false, | ||
is_in_cache: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Property The property Consider adding checks or conditions that ensure this property does not lead to stale data being served, especially in environments with high concurrency or frequent data updates. - is_in_cache: true,
+ is_in_cache: self.check_cache_validity(), Also applies to: 391-391 |
||
}) | ||
} else { | ||
None | ||
|
@@ -388,7 +388,7 @@ impl MappedFile for DefaultMappedFile { | |
start_offset: self.get_file_from_offset() + pos as u64, | ||
size: read_position - pos, | ||
mapped_file: Some(self), | ||
is_in_cache: false, | ||
is_in_cache: true, | ||
}) | ||
} else { | ||
None | ||
|
@@ -588,7 +588,7 @@ impl MappedFile for DefaultMappedFile { | |
}*/ | ||
|
||
fn is_loaded(&self, position: i64, size: usize) -> bool { | ||
todo!() | ||
true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method Setting It's recommended to implement a more robust checking mechanism that verifies the actual load status of the data. - true
+ self.verify_data_integrity()
|
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -437,6 +437,14 @@ | |
} | ||
next_offset | ||
} | ||
|
||
fn check_in_mem_by_commit_offset(&self, offset_py: i64, size: i32) -> bool { | ||
let message = self.commit_log.get_message(offset_py, size); | ||
match message { | ||
None => false, | ||
Some(msg) => msg.is_in_mem(), | ||
} | ||
Comment on lines
+441
to
+446
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Potential issue with The
Analysis chainMethod The implementation uses Rust's pattern matching effectively. Ensure that the Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the implementations of `get_message` and `is_in_mem`.
# Test: Search for the implementations of these methods. Expect: Efficient and correct implementations.
ast-grep --lang rust --pattern $'impl CommitLog {
$$$
get_message($_, $_) {
$$$
}
$$$
}'
ast-grep --lang rust --pattern $'impl Message {
$$$
is_in_mem() {
$$$
}
$$$
}'
Length of output: 1169 Script: #!/bin/bash
# Description: Verify the implementations of `get_message` and `is_in_mem`.
# Corrected pattern for `get_message` method in `CommitLog` implementation.
ast-grep --lang rust --pattern 'impl CommitLog {
fn get_message($_, $_) -> $_ {
$$$
}
}'
# Corrected pattern for `is_in_mem` method in `Message` implementation.
ast-grep --lang rust --pattern 'impl Message {
fn is_in_mem(&self) -> $_ {
$$$
}
}'
Length of output: 192 Script: #!/bin/bash
# Description: Search for the implementations of `get_message` and `is_in_mem` methods across the codebase.
# Search for `get_message` method in the codebase.
rg 'fn get_message'
# Search for `is_in_mem` method in the codebase.
rg 'fn is_in_mem'
Length of output: 1278 Script: #!/bin/bash
# Extract and review the implementation of `get_message` method from commit_log.rs.
rg -A 10 -B 2 'fn get_message' rocketmq-store/src/log_file/commit_log.rs
# Extract and review the implementation of `is_in_mem` method from select_result.rs.
rg -A 10 -B 2 'fn is_in_mem' rocketmq-store/src/base/select_result.rs
Length of output: 1322 |
||
} | ||
} | ||
|
||
fn estimate_in_mem_by_commit_offset( | ||
|
@@ -926,7 +934,28 @@ | |
consume_offset: i64, | ||
batch_size: i32, | ||
) -> bool { | ||
todo!() | ||
let consume_queue = self | ||
.consume_queue_store | ||
.find_or_create_consume_queue(topic, queue_id); | ||
let first_cqitem = consume_queue.lock().get(consume_offset); | ||
if first_cqitem.is_none() { | ||
return false; | ||
} | ||
let cq = first_cqitem.as_ref().unwrap(); | ||
let start_offset_py = cq.pos; | ||
if batch_size <= 1 { | ||
let size = cq.size; | ||
return self.check_in_mem_by_commit_offset(start_offset_py, size); | ||
} | ||
let last_cqitem = consume_queue.lock().get(consume_offset + batch_size as i64); | ||
if last_cqitem.is_none() { | ||
let size = cq.size; | ||
return self.check_in_mem_by_commit_offset(start_offset_py, size); | ||
} | ||
let last_cqitem = last_cqitem.as_ref().unwrap(); | ||
let end_offset_py = last_cqitem.pos; | ||
let size = (end_offset_py - start_offset_py) + last_cqitem.size as i64; | ||
self.check_in_mem_by_commit_offset(start_offset_py, size as i32) | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,7 +205,7 @@ | |
todo!() | ||
} | ||
|
||
fn get(&self, index: i64) -> CqUnit { | ||
fn get(&self, index: i64) -> Option<CqUnit> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Placeholder method The method |
||
todo!() | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,4 +92,8 @@ | |
pub fn destroy(&mut self) { | ||
self.mapped_file_queue.destroy(); | ||
} | ||
|
||
pub fn get(&self, address: i64, cq_ext_unit: &CqExtUnit) -> bool { | ||
unimplemented!() | ||
} | ||
Comment on lines
+96
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Placeholder method The method |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
|
||
use bytes::Buf; | ||
use bytes::BufMut; | ||
use bytes::Bytes; | ||
use bytes::BytesMut; | ||
use rocketmq_common::common::attribute::cq_type::CQType; | ||
use rocketmq_common::common::boundary_type::BoundaryType; | ||
|
@@ -31,6 +32,7 @@ | |
use tracing::warn; | ||
|
||
use crate::base::dispatch_request::DispatchRequest; | ||
use crate::base::select_result::SelectMappedBufferResult; | ||
use crate::base::store_checkpoint::StoreCheckpoint; | ||
use crate::base::swappable::Swappable; | ||
use crate::config::broker_role::BrokerRole; | ||
|
@@ -315,6 +317,20 @@ | |
mapped_file.append_message_bytes(&bytes); | ||
} | ||
} | ||
|
||
pub fn get_index_buffer(&self, start_index: i64) -> Option<SelectMappedBufferResult> { | ||
let mapped_file_size = self.mapped_file_size; | ||
let offset = start_index * CQ_STORE_UNIT_SIZE as i64; | ||
if offset >= self.get_min_logic_offset() { | ||
if let Some(mapped_file) = self | ||
.mapped_file_queue | ||
.find_mapped_file_by_offset(offset, false) | ||
{ | ||
return mapped_file.select_mapped_buffer((offset % mapped_file_size as i64) as i32); | ||
} | ||
} | ||
None | ||
} | ||
Comment on lines
+321
to
+333
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add error handling in Consider adding more robust error handling and validation for out-of-bounds or invalid index values to prevent potential runtime errors. if offset < self.get_min_logic_offset() {
return None; // Early return if offset is out of bounds
} |
||
} | ||
|
||
impl FileQueueLifeCycle for ConsumeQueue { | ||
|
@@ -485,8 +501,11 @@ | |
self.queue_id | ||
} | ||
|
||
fn get(&self, index: i64) -> CqUnit { | ||
todo!() | ||
fn get(&self, index: i64) -> Option<CqUnit> { | ||
match self.iterate_from(index) { | ||
None => None, | ||
Some(value) => None, | ||
} | ||
} | ||
|
||
fn get_cq_unit_and_store_time(&self, index: i64) -> Option<(CqUnit, i64)> { | ||
|
@@ -538,7 +557,7 @@ | |
} | ||
|
||
fn get_min_logic_offset(&self) -> i64 { | ||
todo!() | ||
self.min_logic_offset.load(Ordering::Relaxed) | ||
} | ||
|
||
fn get_cq_type(&self) -> CQType { | ||
|
@@ -776,7 +795,15 @@ | |
} | ||
|
||
fn iterate_from(&self, start_index: i64) -> Option<Box<dyn Iterator<Item = CqUnit>>> { | ||
todo!() | ||
match self.get_index_buffer(start_index) { | ||
None => None, | ||
Some(value) => Some(Box::new(ConsumeQueueIterator { | ||
smbr: Some(value), | ||
relative_pos: 0, | ||
counter: 0, | ||
consume_queue_ext: self.consume_queue_ext.clone(), | ||
})), | ||
} | ||
} | ||
|
||
fn iterate_from_inner( | ||
|
@@ -787,3 +814,66 @@ | |
todo!() | ||
} | ||
} | ||
|
||
struct ConsumeQueueIterator { | ||
smbr: Option<SelectMappedBufferResult>, | ||
relative_pos: i32, | ||
counter: i32, | ||
consume_queue_ext: Option<ConsumeQueueExt>, | ||
} | ||
|
||
impl ConsumeQueueIterator { | ||
fn get_ext(&self, offset: i64, cq_ext_unit: &CqExtUnit) -> bool { | ||
match self.consume_queue_ext.as_ref() { | ||
None => false, | ||
Some(value) => value.get(offset, cq_ext_unit), | ||
} | ||
} | ||
} | ||
|
||
impl Iterator for ConsumeQueueIterator { | ||
type Item = CqUnit; | ||
|
||
fn next(&mut self) -> Option<Self::Item> { | ||
match self.smbr.as_ref() { | ||
None => None, | ||
Some(value) => { | ||
if self.counter * CQ_STORE_UNIT_SIZE >= value.size { | ||
return None; | ||
} | ||
let mmp = value.mapped_file.as_ref().unwrap().get_mapped_file(); | ||
let start = | ||
value.start_offset as usize + (self.counter * CQ_STORE_UNIT_SIZE) as usize; | ||
self.counter += 1; | ||
let end = start + CQ_STORE_UNIT_SIZE as usize; | ||
let mut bytes = Bytes::copy_from_slice(&mmp[start..end]); | ||
let pos = bytes.get_i64(); | ||
let size = bytes.get_i32(); | ||
let tags_code = bytes.get_i64(); | ||
let mut cq_unit = CqUnit { | ||
queue_offset: start as i64 / CQ_STORE_UNIT_SIZE as i64, | ||
size, | ||
pos, | ||
tags_code, | ||
..CqUnit::default() | ||
}; | ||
|
||
if ConsumeQueueExt::is_ext_addr(cq_unit.tags_code) { | ||
let cq_ext_unit = CqExtUnit::default(); | ||
let ext_ret = self.get_ext(cq_unit.tags_code, &cq_ext_unit); | ||
if ext_ret { | ||
cq_unit.tags_code = cq_ext_unit.tags_code(); | ||
cq_unit.cq_ext_unit = Some(cq_ext_unit); | ||
} else { | ||
error!( | ||
"[BUG] can't find consume queue extend file content! addr={}, \ | ||
offsetPy={}, sizePy={}", | ||
cq_unit.tags_code, cq_unit.pos, cq_unit.pos, | ||
); | ||
} | ||
} | ||
Some(cq_unit) | ||
} | ||
} | ||
} | ||
} | ||
Comment on lines
+818
to
+879
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor The iterator should robustly handle cases where the underlying data might be modified during iteration, preventing inconsistent states or crashes. if let Some(mmp) = value.mapped_file.as_ref() {
// Proceed with existing logic
} else {
error!("Mapped file missing during iteration.");
return None;
} |
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.
Review the logic of
is_in_mem
method.The method
is_in_mem
uses a match statement to determine if the mapped file is in memory based on thestart_offset
. The logic seems correct, but the return value whenmapped_file
isNone
is set totrue
, which might be incorrect as it assumes the buffer is in memory when there's no associated file. This could lead to potential bugs.Committable suggestion