slbotbm commented on PR #3046:
URL: https://github.com/apache/iggy/pull/3046#issuecomment-4150411892

   @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 
   ```rust
   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 `Message.new_message(...)` from 
the cpp side while avoiding ambiguity.
   
   ---
   `GetStreamsReturnsEmptyAfterCleanup` and 
`GetStreamsReturnsStreamAfterCreation` test the `get_streams` function, but 
they are in the `messages/` folder. Please move them to their appropriate 
fields.
   
   ---
   The amount of testing is insufficient for the three functions.
   Please think about the following and add tests:
   - `get_streams`:
       - For `GetStreamsReturnsStreamAfterCreation`, verify everything that is 
verifiable, not just the name. Also create additional streams and topics inside 
them, and verify everything.
       - Does the sdk allow calling `get_streams` before `login` and `connect`?
       - Does calling `get_stream` and `get_streams` when there is only one one 
stream return the same same with the same details?
       - What happens when you call `get_streams` repeatedly?
       - Lastly, in `GetStreamsReturnsEmptyAfterCleanup`, you are deleting 
every stream without ever creating any. Instead of this, let's create some 
unique streams, delete those, and then verify whether 0 is returned or not.
   
   - `send_messages`:
       - What happens when you call send_messages before login and connect?
       - What happens when you give invalid topic and / or stream identifiers 
to the function?
       - What happens when you give wrong topic and / or stream identifiers to 
the function?
       - What happens when you give wrong partitioning kind and / or value to 
the function? (also test the relevant limits)
       - Verify that the messages are being stored in partitions according to 
the strategy we have strategy we have chosen.
       - What happens when you supply an empty vector to the `send_messages`? 
       - Is there a limit on how many messages can be sent in total, or in a 
single batch?
       - What happens when you try to send an invalid payload through 
`send_messages` (empty payload, payload size > 64,000,000 bytes)
       - When `send_messages` is called multiple times, is the order of the 
messages preserved? 
       - You have written a test where one custom ID is set and sent to the 
server. What happens when you set the same ID for multiple messages?
       - Lastly, since we are working with bytes, I would like to you to try 
sending a variety of payloads, and check whether they succeed or not. (null 
bytes, utf-8 strings, emojis, etc. - consider using property based testing)
   - `poll_messages` :
       - What happens when you call poll_messages before login and connect?
       - What happens when you give invalid topic, stream, and / or consumer 
identifiers to the function?
       - What happens when you give wrong topic, stream, and / or consumer 
identifiers to the function?
       - What happens when you give a wrong consumer_kind to the function?
       - What happens when you give a wrong polling strategy kind and / or 
value to the function? (also test the relevant limits)
       - What happens when you give an invalid or non-existent partition ID to 
the function?
       -  What happens when you supply count = 0 to the poll_messages?
       - Is there a limit on how many messages can be requested in a single 
poll?
       - When the requested count is smaller than the number of available 
messages, are only that many messages returned?
       - When the requested count is larger than the number of available 
messages, are only the available messages returned?
       - Verify that polling without specifying a partition behaves as expected.
       - Verify the behavior of each polling strategy: offset, first, last, 
next, and timestamp.
       - For offset, what happens when the offset is valid, too large, or 
points exactly to the end?
       - For timestamp, what happens when the timestamp is before the first 
message, between messages, and after the last message?
       - What happens when auto_commit = false and poll_messages is called 
repeatedly with next?
       - What happens when auto_commit = true and poll_messages is called 
repeatedly with next?
       - Are offsets tracked independently for different consumer IDs?
       - Are offsets tracked independently for consumer and consumer_group?
       - When multiple consumers in the same consumer group poll, is the 
behavior what we expect?
       - When poll_messages is called multiple times after multiple 
send_messages calls, is the order of the returned messages preserved?
       - Verify that the returned offsets are monotonic and correct across 
multiple polls.
       - Verify that the returned payloads are preserved exactly for a variety 
of byte payloads. (null bytes, utf-8 strings, emojis, arbitrary bytes, etc. - 
consider doing property-based testing)
       - Verify that custom message IDs, if present, are returned correctly for 
multiple messages, not just one.
       - What happens when you poll from an empty partition, and then poll 
again after new messages are sent?
       - What happens when you poll after the stream or topic has been deleted?
   


-- 
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]

Reply via email to