slbotbm commented on PR #3046:
URL: https://github.com/apache/iggy/pull/3046#issuecomment-4149899176
@seokjin0414 Thanks for the PR!
Here is my first round of review:
The CI is failing. Please check which test is failing. Also, the cpp code is
likely not linted properly, so please do that as well.
---
Let's unify the Message struct, instead of having separate structs for
sending and polling. The unified struct could look like:
```rust
struct Message {
checksum: u64,
id_lo: u64,
id_hi: u64,
offset: u64,
timestamp: u64,
origin_timestamp: u64,
user_headers_length: u32,
payload_length: u32,
reserved: u64,
payload: Vec<u8>,
user_headers: Vec<u8>,
}
```
Define a function using `impl Message` that would allow the user to create a
message to send. This has to be exported to the cpp side. If the bdd tests only
require the payload to be set, you can just define a function called
`payload(Vec<u8>)`.
---
You are using `map(Into::into)` in many places. Let's use `from` to be more
explicit about what is being converted to what.
---
```rust
let mut iggy_messages: Vec<IggyMessage> = messages
.into_iter()
.map(|m| {
let id = ((m.id_hi as u128) << 64) | (m.id_lo as u128);
let payload = Bytes::from(m.payload);
let msg = if id > 0 {
IggyMessage::builder().id(id).payload(payload).build()
} else {
IggyMessage::builder().payload(payload).build()
};
msg.map_err(|error| format!("Could not build message:
{error}"))
})
.collect::<Result<Vec<_>, _>>()?;
```
In the above code, please define `TryFrom<ffi::Message> for IggyMessage` for
the conversion and use that.
---
Let's rename `strategy_kind` and `strategy_value` to
`partitioning_strategy_kind` and `partitioning_strategy_value` so that there is
no ambiguity about its meaning.
--
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]
