hubcio commented on PR #2360: URL: https://github.com/apache/iggy/pull/2360#issuecomment-3546396770
hi, thanks for contribution! good stuff. first of all, i think as of now you can totally ignore quic. let's focus solely on tcp. answering your questions: 1. i've never written anything in lua, so I cannot say. maybe you can look at other popular application-level protocols like kafka/redis/zeromq and check how they are structured, and use it as an inspiration? 2. yes, iggy works on request-response mechanism, meaning that between two peers (server and client) there can not be out of order message - client won't send next request if response for previous one hasn't arrived 3. I think tshark testing is corect approach because you're essentially writing wireshark/tshark plugin, you don't want to test rust (de)serialization. but i think we can improve it: check if you can use https://crates.io/crates/rtshark which would reduce binary dependencies. my general thoughts: * about maintainability - this is my main concern. you'll need to implement 47+ commands, each with request/response dissectors. a few questions: - are you planning to maintain this long-term? protocol evolves and we'd need dissector updates (this is bait question, see last point here) - how do we ensure dissector stays in sync with protocol? there's no automated verification like the SDK has with `BytesSerializable` - for such a large file (will be 2000+ lines), consider a code generator approach - parse the command definitions from rust source and generate lua? this would reduce maintenance burden significantly, e.g. https://docs.rs/wsdf/latest/wsdf/ but keep in mind that some of our structs do not match 1:1 the protocol, e.g. `SendMessages` * about tests: instead of cargo test -p wireshark --features protocol-tests requiring manual server start, please integrate with our test infrastructure in core/integration. look at how we spawn test servers there - you can get a fresh server instance per test. this also eliminates port conflicts and cleanup issues. lastly, I found some bug, steps to reproduce: 1. start server via `cargo build && target/debug/iggy-server --fresh --with-default-root-credentials` 2. in other shell start wireshark with interface lo via `sudo tcpdump -i lo -w /tmp/iggy.pcap` 3. in other shell start bench which send 50 MB of data `target/debug/iggy-bench -T 50MB pp -p 1 tcp` 5. stop tcpdump and open `/tmp/iggy.pcap in wireshark` result: lua out of bounds for command 302 (create topic) overall good foundation, just need to think about long-term maintenance story before we commit to this in main repo. -- 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]
