slbotbm commented on code in PR #3755:
URL: https://github.com/apache/iggy/pull/3755#discussion_r3659252567


##########
foreign/cpp/Cargo.toml:
##########
@@ -17,7 +17,7 @@
 
 [package]
 name = "iggy-cpp"
-version = "0.1.1"
+version = "0.1.2"

Review Comment:
   It would be good to update the bazel version as well.



##########
foreign/cpp/src/client.rs:
##########
@@ -37,6 +37,18 @@ use std::sync::Arc;
 /// reserve `u32::MAX` as the sentinel for `partition_id`.
 const ANY_PARTITION_ID: u32 = u32::MAX;
 
+/// A C++ `enum class` can hold any value of its underlying type, so cxx models
+/// a shared enum as an open repr and the catch-all arm is reachable.
+fn resolve_binary_request_kind(
+    kind: ffi::BinaryRequestKind,
+) -> Result<RustBinaryRequestKind, String> {

Review Comment:
   Let's convert this to `TryFrom<ffi::BinaryRequestKind> for 
RustBinaryRequestKind` in type_conversion.rs



##########
bdd/cpp/features/step_definitions/raw_command_steps.cpp:
##########
@@ -25,18 +25,35 @@
 
 #include <cstdint>
 #include <exception>
+#include <stdexcept>
 #include <string>
 
 #include "world.hpp"
 
-WHEN("^I send a raw command with code ([0-9]+) and an empty payload$") {
+namespace {
+
+iggy::ffi::BinaryRequestKind parse_request_kind(const std::string &kind) {
+    if (kind == "non_replicated") {
+        return iggy::ffi::BinaryRequestKind::NonReplicated;
+    }
+    if (kind == "replicated") {
+        return iggy::ffi::BinaryRequestKind::Replicated;
+    }
+    throw std::invalid_argument("unknown binary request kind: " + kind);

Review Comment:
   Let's inline this since there is only 1 call site.



##########
foreign/cpp/Cargo.toml:
##########
@@ -26,6 +26,12 @@ ignored = ["cxx-build"]
 [lib]
 crate-type = ["staticlib"]
 
+# Framing is a compile-time choice, so the static library is locked to 
whatever it
+# was built with. Opt in with `--features vsr` to build against the consensus
+# protocol; the default build stays on the classic protocol.
+[features]
+vsr = ["iggy/vsr", "iggy_common/vsr"]

Review Comment:
   We utilize bazel as the build system, and we'll need to add this feature 
flag there as well. 



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