feat(cpp): add messaging FFI functions for C++ SDK#3046
feat(cpp): add messaging FFI functions for C++ SDK#3046seokjin0414 wants to merge 21 commits intoapache:masterfrom
Conversation
|
@seokjin0414 Thanks for the PR! Here is my first round of review: Let's unify the Message struct, instead of having separate structs for sending and polling. The unified struct could look like: 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 You are using 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 Let's rename |
|
@slbotbm Thanks for the review! Addressed all feedback in c8f9adf:
|
|
@seokjin0414 Thanks for the quick turn-around. I may not have been clear in my review previously, but I wanted you to implement something more like impl ffi::Message {
pub fn new_message(&mut self, payload: Vec<u8>) -> Result<(), String> {
if payload.is_empty() {
return Err("Could not create message: payload must not be empty".to_string());
}
let payload_length = payload.len() as u32;
*self = Self {
checksum: 0,
id_lo: 0,
id_hi: 0,
offset: 0,
timestamp: 0,
origin_timestamp: 0,
user_headers_length: 0,
payload_length,
reserved: 0,
payload,
user_headers: Vec::new(),
};
Ok(())
}
}The above interface would allow us to call
The amount of testing is insufficient for the three functions.
|
|
I wrote the above in multiple sittings, so please tell me if I wrote overlapping things. |
|
One comment from my side: remember that Rust SDK (which you are calling) is also perform these validations and returns respective errors. Make sure that you are not double checking this on rust wrapper side or cpp side. |
e6ba6e9 to
0a49445
Compare
|
|
@seokjin0414 Thanks for the update. Could you please tell me which tests you have not added from the above list? Just copy pasting the questions I had written will do. |
|
@hubcio about your comment: would it make sense to add some of these test to the rust sdk instead? |
|
@slbotbm send_messages:
poll_messages:
I can add the overlooked ones now. |
|
@seokjin0414 please go ahead and add the ones you overlooked. I'll review then |
c8d885a to
8742221
Compare
|
@slbotbm
Still not added:
|
|
@seokjin0414 Thanks for the update! I'll review this by friday/saturday. In the meantime, could you please:
|
0d16d27 to
8b74d12
Compare
acb69ac to
472616e
Compare
|
There was a problem hiding this comment.
@seokjin0414 I requested some small changes.
Other than this, I noticed
- you were not testing cases for
send_messagesandpoll_messageswhen invalid topic ID was passed to them. - No test for invalid consumer identifier for
poll_messages
Also, since the consumer group PR was merged, could you add a test for creating a consumer group and polling messages after joining that?
b889509 to
2b22956
Compare
Signed-off-by: shin <sars21@hanmail.net>
…d struct Signed-off-by: shin <sars21@hanmail.net>
…PolledMessages Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
…sages Signed-off-by: shin <sars21@hanmail.net>
- fix error message format consistency in send_messages partitioning - extract to_payload and partition_id_bytes helpers to test_helpers.hpp - fix GetStreamsReturnsEmpty test to clean up before asserting Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
…rove naming - unify IggyMessageToSend/IggyMessagePolled into single Message struct - add new_message(payload) factory function exported to C++ - add TryFrom<ffi::Message> for IggyMessage conversion - replace map(Into::into) with explicit map(ffi::X::from) - rename strategy_kind/value to polling_strategy_kind/value Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
…method Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
…, and poll_messages Signed-off-by: shin <sars21@hanmail.net>
…t, timestamp, and offset verification Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
…ertions, remove redundant test - Rename 10 error-throwing tests with `Throws` suffix for naming consistency - Add payload/offset verification to 7 tests that only checked counts - Delete PollMessagesLargeCount test (not explicitly testing limits) Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
- Add join_consumer_group and leave_consumer_group FFI functions - Add field assertions to GetStreamsReturnsStreamAfterCreation test - Add field comparison to GetStreamsConsistentWithGetStream test - Add SendMessagesWithInvalidTopicIdThrows test - Add PollMessagesWithInvalidTopicIdThrows test - Add PollMessagesWithInvalidConsumerIdThrows test - Add ConsumerGroupCreateJoinAndPollMessages e2e test Signed-off-by: shin <sars21@hanmail.net>
2b22956 to
41a3f03
Compare
seokjin0414
left a comment
There was a problem hiding this comment.
Thank you for the review!
- Added field assertions to
GetStreamsReturnsStreamAfterCreation(created_at,size_bytes,messages_count,topics_count) and field comparison toGetStreamsConsistentWithGetStream(created_at,size_bytes). Note:idis excluded from both due to a known cxxVec<SharedStruct>issue whereidreturns 0. - Added
SendMessagesWithInvalidTopicIdThrows,PollMessagesWithInvalidTopicIdThrows,PollMessagesWithInvalidConsumerIdThrows - Added
join_consumer_groupandleave_consumer_groupFFI functions - Added
ConsumerGroupCreateJoinAndPollMessagestest covering the full flow: create → join → verify member count → send → poll with consumer_group kind → verify payloads → leave → verify member count drops to 0
There was a problem hiding this comment.
@seokjin0414 Thanks for the update!
Could you add the following comment in tests/client/low_level_e2e.cpp please:
TODO(slbotbm): Add tests for join_consumer_group() and leave_consumer_group()
Also, you commented:
Note: id is excluded from both due to a known cxx Vec issue where id returns 0.
Do you have an issue you could link to for me to see?
Otherwise lgtm!
Signed-off-by: shin <sars21@hanmail.net>
Signed-off-by: shin <sars21@hanmail.net>
|
@slbotbm |
Summary
Closes #2965 (Phase 1 of 2).
get_streams,send_messages,poll_messagesFFI functions to C++ SDKStream,IggyMessageToSend,IggyMessagePolled,PolledMessagesDiscussed with @slbotbm on Discord — PR split and message struct design confirmed.
Test plan
cargo check/cargo clippy/cargo fmtcleantests/message/low_level_e2e.cpp(Bazel + iggy-server)