Feature/raft #15
Feature/raft #15
Conversation
- Framework core components in-progress - KVStore interface & implementation added - Config for the entire framework added
- Add interface for scenarios - Implement CRUD scenario
- Add integration test executor
- Add test orchestrator
- Implement simple CRUD testing via kvtest - Fix memtables not being flushed
- Leftover cleanup
- Ignore binaries
- ReadYourWrites scenario in-progress
- ReadYourWrites scenario in-progress
- Implement basic memtable stress test - General refactoring
- Fix Raft unit tests - Add mise support
- Improve logging - Simplify compaction threshold calculation - Update infra files
- Project restructure
- Fixed bugs in serialization layer - Introduced mmaping for record retrieval - Segment flush & flush now uses the new serialization layer
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughComprehensive refactoring restructuring the codebase from a lib/ based architecture to src/ with reorganized subdirectories, introducing serialization framework, UUID utilities, and Go-based integration testing, while updating core APIs (LSMTree put, Memtable record structure), dependencies, and build configurations. Changes
Sequence Diagram(s)Conditions for sequence diagram generation are not met for this change set. While the PR introduces new components (serialization, test framework) with multiple interactions, the changes are too heterogeneous and span too many disparate systems (build config, API refactoring, new frameworks, infrastructure updates) to form a coherent sequential flow. The architectural shifts lack a single primary interaction pattern suitable for visualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (18)
src/server/grpc_server.cpp (4)
39-42: Improve exception handling for better observability.The exception handling has several issues:
- Exception is caught by non-const reference (should be
const std::exception &e)- Exception message is not logged, making debugging difficult
- Returning
CANCELLEDstatus is misleading for unexpected exceptions; considerINTERNALorUNKNOWN🔎 Proposed fix
- catch (std::exception &e) + catch (const std::exception &e) { + spdlog::error("Exception in Put operation: {}", e.what()); - return grpc::Status::CANCELLED; + return grpc::Status{grpc::StatusCode::INTERNAL, e.what()}; }
60-63: Improve exception handling for better observability.Same exception handling issues as in the
Putmethod:
- Exception is caught by non-const reference
- Exception message is not logged
- Returning
CANCELLEDstatus is misleading🔎 Proposed fix
- catch (std::exception &e) + catch (const std::exception &e) { + spdlog::error("Exception in Get operation: {}", e.what()); - return grpc::Status::CANCELLED; + return grpc::Status{grpc::StatusCode::INTERNAL, e.what()}; }
77-77: Replaceexit()calls with exception-based error handling.Using
exit(EXIT_FAILURE)in library/server code prevents graceful error handling, cleanup, and recovery. This is particularly problematic for server components that should handle errors without terminating the process.Consider throwing exceptions or returning error codes that allow callers to handle failures appropriately.
Also applies to: 83-83, 90-90, 110-110, 119-119
127-127: Add null check before dereferencingm_server.The
shutdown()method dereferencesm_serverwithout checking if it's null. Ifshutdown()is called when the server hasn't been started or after a previous shutdown, this will cause undefined behavior.🔎 Proposed fix
try { + if (!m_server) + { + spdlog::warn("gRPC server is not running, nothing to shutdown"); + return; + } m_server->Shutdown(); }src/structures/lsmtree/segments/lsmtree_segment_factory.cpp (1)
6-10: Fix parameter order in factory function signature to match constructor expectations.The factory function signature declares parameters as
name, path, memtable, but passes them to the constructor aspath, name, memtable. This creates a critical mismatch: callers using positional arguments will havenameandpathvalues silently swapped when constructing the segment. Change the signature tolsmtree_segment_factory(fs::path_t path, types::name_t name, memtable::memtable_t memtable)to match the constructor's expected parameter order.src/include/server/server_kind.h (1)
13-17: Remove obsoletetcp_kenum value.The enum retains
tcp_keven though TCP support has been removed (TCP_STR_VIEW removed, TCP factory specialization removed per context). This creates dead code and will cause compilation failures iftcp_kis passed to the factory function (no specialization exists for it).🔎 Suggested fix
enum class communication_strategy_kind_k : uint8_t { - tcp_k, grpc_k };Additionally, verify and update the
to_stringandfrom_stringfunction implementations to remove tcp handling.#!/bin/bash # Verify if tcp_k is still referenced elsewhere in the codebase # Search for references to tcp_k enum value rg -n 'tcp_k' --type=cpp --type=h -g '!server_kind.h' # Search for string "tcp" in server-related context rg -n '"tcp"' src/include/server/ src/lib/server/ --type=cpp --type=h # Check the implementation of to_string and from_string functions fd -e cpp 'server_kind' src/ --exec cat {}src/include/server/server_factory.h (1)
14-21: Critical: Factory template incompatible with enum definition.The factory function signature (line 23) accepts
communication_strategy_kind_kas a template parameter, but this enum includestcp_k(see relevant snippet from server_kind.h lines 13-16). However, only thegrpc_communication_tspecialization exists (line 18). Attempting to instantiatefactory<communication_strategy_kind_k::tcp_k>()will fail at compile time with no matching specialization.This issue is coupled with the
tcp_kenum value retention in server_kind.h that should be removed as part of TCP desupport.🔎 Recommended fixes
Option 1 (Recommended): Remove
tcp_kfrom the enum in server_kind.h (already flagged in that file's review).Option 2 (Defense-in-depth): Add a static_assert to catch misuse at compile time:
template <communication_strategy_kind_k kind> [[nodiscard]] auto factory() noexcept { + static_assert(kind == communication_strategy_kind_k::grpc_k, + "Only gRPC communication strategy is supported"); try { return typename CommunicationFactory<kind>::type();#!/bin/bash # Check if tcp_k is used anywhere with the factory # Search for factory instantiations rg -n 'factory<.*tcp' --type=cpp --type=h # Search for main_server calls that might use tcp_k rg -n 'main_server.*tcp' --type=cpp --type=h # Look for any enum usage of tcp_k rg -n 'communication_strategy_kind_k::tcp_k' --type=cpp --type=hAlso applies to: 23-34
src/db/db.cpp (1)
356-364: Data race:requestFailedrequiresWriterMutexLock, but the lock acquired isReaderMutexLock.
requestFailed()modifiesm_pendingRequests(erases entries and sets promise values), but line 358 acquires aReaderMutexLock. This causes undefined behavior when multiple threads attempt to modify the map concurrently.🔎 Proposed fix
if (!timedOut.empty()) { - absl::ReaderMutexLock locker{&m_pendingMutex}; + absl::WriterMutexLock locker{&m_pendingMutex}; for (auto id : timedOut) { requestFailed(id); } }src/include/structures/skiplist/skiplist.h (1)
178-196: Doublestd::forwardon the same variable causes undefined behavior.In the insert path (line 183),
recordis forwarded (potentially moved from). If the key already exists, the code falls through to line 192 where it attempts to forward the samerecordagain at line 195. Ifrecordwas moved in the first branch, this second forward operates on a moved-from object.🔎 Proposed fix
Since the two branches are mutually exclusive (
current == nullptr || current->record.m_key != record.m_keyvscurrent->record.m_key == record.m_key), the double forward won't actually happen at runtime. However, theelse ifcondition is redundant given the previousif. Consider restructuring for clarity:current = current->forward[0]; if (current == nullptr || current->record.m_key != record.m_key) { // Insert new node node_shared_ptr_t newNode = std::make_shared<node_t>(std::forward<TRecord>(record), m_level); for (std::int64_t i{0}; i <= newLevel; i++) { newNode->forward[i] = to_be_updated[i]->forward[i]; to_be_updated[i]->forward[i] = newNode; } m_size++; } - else if (current->record.m_key == record.m_key) + else { // Update value of the existing node current->record = std::forward<TRecord>(record); }src/include/concurrency/thread_safe_queue.h (2)
71-81: Move constructor does not transferm_reservedIndex.The move constructor only transfers
m_queuebut leavesm_reservedIndexat its default (nullopt). If the source had an active reservation, it will be silently lost, and the source will retain a stale reservation state.🔎 Proposed fix
template <typename TItem> inline thread_safe_queue_t<TItem>::thread_safe_queue_t(thread_safe_queue_t &&other) noexcept - : m_queue{ + : m_reservedIndex{ + [&other]() -> std::optional<std::uint64_t> + { + absl::WriterMutexLock lock{&other.m_mutex}; + auto idx = std::move(other.m_reservedIndex); + other.m_reservedIndex = std::nullopt; + return idx; + }() + }, + m_queue{ [&other]() -> queue_t { absl::WriterMutexLock lock{&other.m_mutex}; return std::move(other.m_queue); } - } + }() { }Note: The current implementation also has a potential issue with acquiring the lock twice (once per lambda). Consider refactoring to a single lock acquisition.
83-93: Move assignment does not transferm_reservedIndex.Similar to the move constructor, the move assignment operator only transfers
m_queue, losing any active reservation state.🔎 Proposed fix
template <typename TItem> inline auto thread_safe_queue_t<TItem>::operator=(thread_safe_queue_t &&other) noexcept -> thread_safe_queue_t & { if (this != &other) { absl_dual_mutex_lock_guard lock{m_mutex, other.m_mutex}; m_queue = std::move(other.m_queue); + m_reservedIndex = std::move(other.m_reservedIndex); + other.m_reservedIndex = std::nullopt; } return *this; }src/include/structures/memtable/memtable.h (2)
242-256:m_sequenceNumberis not serialized inwrite()or deserialized inread().The
record_t::write()andrecord_t::read()templates serialize/deserializem_key,m_value, andm_timestamp, but omitm_sequenceNumber. This will cause data corruption when records are persisted and later read back, as the sequence number will be lost.🔎 Proposed fix
template <typename TSTream> void memtable_t::record_t::write(TSTream &outStream) const { m_key.write(outStream); outStream << ' '; m_value.write(outStream); outStream << ' '; + outStream << m_sequenceNumber << ' '; m_timestamp.write(outStream); } template <typename TSTream> void memtable_t::record_t::read(TSTream &outStream) { m_key.read(outStream); m_value.read(outStream); + outStream >> m_sequenceNumber; m_timestamp.read(outStream); }
232-237: Type mismatch betweenwrite()andread()for timestamp.
write()casts tostd::uint64_t, butread()usesclock_t::rep(which isstd::int64_tfor most implementations). Additionally,read()reconstructs usingclock_t::time_pointandclock_t::duration, but the timestamp usesprecision_t(nanoseconds), not the clock's native duration.🔎 Proposed fix
template <typename TSTream> void memtable_t::record_t::timestamp_t::write(TSTream &outStream) const { - outStream << static_cast<std::uint64_t>(m_value.time_since_epoch().count()); + outStream << m_value.time_since_epoch().count(); } template <typename TSTream> void memtable_t::record_t::timestamp_t::read(TSTream &outStream) { - clock_t::rep count = 0; + precision_t::rep count = 0; outStream >> count; - m_value = clock_t::time_point{clock_t::duration{count}}; + m_value = time_point_t{precision_t{count}}; }src/structures/lsmtree/levels/level.cpp (2)
46-46: Redundant doublestd::moveon manifest.
std::move(std::move(manifest))is redundant. The innerstd::movealready converts to an rvalue reference; the outer one has no additional effect and reduces readability.Proposed fix
- m_manifest{std::move(std::move(manifest))} + m_manifest{std::move(manifest)}
159-165: Comment mentions timestamp-based deduplication, but logic only checks key equality.The comment states "choose the one with the greatest timestamp," but the code only checks
lastKey != current.first->m_key. If duplicate keys exist across segments with different timestamps, the first one encountered wins, not the one with the greatest timestamp. Either update the comment to reflect actual behavior or implement timestamp comparison.src/include/config/config.h (1)
28-31:std::forwardmissing template argument - compilation error.
std::forward(args)...will not compile.std::forwardrequires an explicit template argument to properly forward the value category.Proposed fix
template <typename... Args> auto make_shared(Args... args) { - return std::make_shared<config_t>(std::forward(args)...); + return std::make_shared<config_t>(std::forward<Args>(args)...); }Additionally,
Args... argsshould beArgs&&... argsfor perfect forwarding:-template <typename... Args> auto make_shared(Args... args) +template <typename... Args> auto make_shared(Args&&... args)src/config/config.cpp (2)
244-244: Wrong format specifier.
fmt::formatuses{}placeholders, not%s. This will output the literal%sinstead of the logging level.🔎 Proposed fix
- throw std::runtime_error(fmt::format("Unknown logging level: %s", loggingLevel)); + throw std::runtime_error(fmt::format("Unknown logging level: {}", loggingLevel));
438-441: Wrong error message.The error message says
"id"but this block handles the missing"peers"field.🔎 Proposed fix
if (configJson.contains("peers")) { dbConfig->ServerConfig.peers = configJson["peers"].get<std::vector<std::string>>(); } else { - throw std::runtime_error("\"id\" is not specified in the config"); + throw std::runtime_error("\"peers\" is not specified in the config"); }
🟠 Major comments (24)
src/include/structures/lsmtree/segment_serializer.h-1-21 (1)
1-21: Remove or complete the unused segment_serializer header and implementation files.The header and corresponding implementation file (
segment_serializer.cpp) are completely empty scaffolding that is not referenced anywhere in the codebase. The header contains eleven includes (standard library,spdlog,magic_enum, and internal serialization/segment headers) with no symbols used within the empty namespace. The.cppfile is also empty (0 bytes).If segment serialization helpers were planned to support the
flush()andrestore()methods inregular_segment_t, either complete these utilities or remove the unused files to avoid confusion and maintain a clean codebase.tests/go/kvtest/proto/raft/v1/raft_service.proto-24-28 (1)
24-28: Address the skipped field number in LogEntry.Field number 3 is skipped (jumps from 2 to 4). If this was intentional to reserve the number for future use or due to a removed field, explicitly mark it as reserved for clarity:
reserved 3;Otherwise, renumber the
payloadfield to use field number 3.🔎 Proposed fix to use field number 3
message LogEntry { uint32 term = 1; uint32 index = 2; - bytes payload = 4; + bytes payload = 3; }tests/go/kvtest/proto/raft/v1/raft_service.proto-55-59 (1)
55-59: Change vote_granted to bool type.In standard Raft,
vote_grantedindicates whether a vote was granted (true) or denied (false). Usinguint32instead ofboolis non-standard and error-prone. This should be changed toboolfor clarity and correctness.🔎 Proposed fix
message RequestVoteResponse { uint32 term = 1; - uint32 vote_granted = 2; + bool vote_granted = 2; uint32 responder_id = 3; }tests/go/kvtest/proto/raft/v1/raft_service.proto-30-38 (1)
30-38: Remove redundant sender_id field.In standard Raft, the sender of AppendEntries is always the leader. Having both
leader_id(field 2) andsender_id(field 7) creates redundancy and potential inconsistency. Additionally, there's a type mismatch:leader_idis astringwhilesender_idisuint32.Consider removing
sender_idor clarifying why both are needed with documentation.🔎 Proposed fix to remove sender_id
message AppendEntriesRequest { uint32 term = 1; string leader_id = 2; uint32 prev_log_index = 3; uint32 prev_log_term = 4; repeated LogEntry entries = 5; uint32 leader_commit = 6; - uint32 sender_id = 7; }tests/go/kvtest/proto/raft/v1/raft_service.proto-61-65 (1)
61-65: Use bytes instead of string for payloads.The
payloadsfield usesrepeated string, which requires valid UTF-8 encoding. If payloads contain arbitrary binary data (which is typical for log entries), userepeated bytesinstead to avoid encoding issues.🔎 Proposed fix
message ReplicateRequest { uint32 term = 1; uint32 sender_id = 2; - repeated string payloads = 3; + repeated bytes payloads = 3; }tests/go/kvtest/proto/raft/v1/raft_service.proto-47-53 (1)
47-53: Remove redundant sender_id field and address field numbering.In standard Raft, the sender of RequestVote is always the candidate. Having both
candidate_id(field 2) andsender_id(field 7) is redundant. Additionally, field numbering jumps from 4 to 7, skipping 5 and 6.Consider removing
sender_idor marking fields 5-6 as reserved if they were previously used.🔎 Proposed fix to remove sender_id and adjust numbering
message RequestVoteRequest { uint32 term = 1; uint32 candidate_id = 2; uint32 last_log_index = 3; uint32 last_log_term = 4; - uint32 sender_id = 7; }tests/go/kvtest/pkg/scenarios/crud.go-58-60 (1)
58-60: Implement or disable delete and exists operations.These operations return "operation not implemented" errors, which will cause test failures when they're randomly selected (lines 101-108). Consider either:
- Implementing the operations
- Removing them from the operation selection logic until they're ready
Would you like me to generate implementations for these operations?
tests/go/kvtest/pkg/core/kvstore.go-34-39 (1)
34-39: Use appropriate error types for predefined KV errors.The predefined errors use semantically incorrect underlying error types:
ErrKeyNotFoundusescontext.DeadlineExceeded, which suggests a timeout occurredErrPutFailedandErrDeleteFailedusecontext.Canceled, which suggests context cancellationThese context errors don't accurately represent the domain-specific failures. Consider using
errors.New()or custom sentinel errors instead.🔎 Proposed fix
+import ( + "context" + "errors" +) + // Predefined errors var ( - ErrKeyNotFound = &KVError{Op: "get", Err: context.DeadlineExceeded} - ErrPutFailed = &KVError{Op: "put", Err: context.Canceled} - ErrDeleteFailed = &KVError{Op: "delete", Err: context.Canceled} + ErrKeyNotFound = &KVError{Op: "get", Err: errors.New("key not found")} + ErrPutFailed = &KVError{Op: "put", Err: errors.New("put operation failed")} + ErrDeleteFailed = &KVError{Op: "delete", Err: errors.New("delete operation failed")} )Committable suggestion skipped: line range outside the PR's diff.
tests/go/kvtest/pkg/scenarios/crud.go-94-110 (1)
94-110: Clarify the operation selection logic.The TODOs and comments in this function are confusing:
- Lines 102-104: Comment says "Should be 'exists'" but returns "delete"
- Lines 106-108: Comment says "Should be 'exists'" but returns "exists"
The probability distribution also seems incomplete:
- Read: < readRatio
- Write: < readRatio + writeRatio
- Delete: < readRatio + writeRatio + 0.1 (hardcoded 10%)
- Exists: everything else
This means if readRatio (0.7) + writeRatio (0.3) = 1.0, delete and exists will never be selected. Clarify the intended behavior and either fix the logic or remove unimplemented operations.
🔎 Suggested fix to remove unimplemented operations temporarily
func (s *CRUDScenario) selectOperation() string { r := rand.Float64() if r < s.readRatio { return "read" - } else if r < s.readRatio+s.writeRatio { - return "write" - } else if r < s.readRatio+s.writeRatio+0.1 { - // TODO(lnikon): Should be "exists" - // return "delete" - return "delete" } else { - // TODO(lnikon): Should be "exists" - // return "exists" - return "exists" + return "write" } }tests/go/kvtest/pkg/adapters/tinykvpp.go-109-118 (1)
109-118: HealthCheck silently ignores errors.The
Getcall result is discarded, so network failures, timeouts, or server errors go unnoticed. This defeats the purpose of a health check.🔎 Proposed fix
func (a *TinyKVPPAdapter) HealthCheck(ctx context.Context) error { if a.conn == nil { return fmt.Errorf("not connected") } testKey := "__health_check__" - a.client.Get(ctx, &pb.GetRequest{Key: testKey}) - - return nil + _, err := a.client.Get(ctx, &pb.GetRequest{Key: testKey}) + if err != nil { + return fmt.Errorf("health check failed: %w", err) + } + return nil }tests/go/kvtest/pkg/scenarios/readyourwrites.go-26-32 (1)
26-32: Same type assertion issue forintparameters.Similar to
memtable_stress.go, YAML-parsed numbers are typicallyfloat64.🔎 Proposed fix
- if val, ok := params["value_size"].(int); ok { - scenario.valueSize = val + if val, ok := params["value_size"].(float64); ok { + scenario.valueSize = int(val) + } else if val, ok := params["value_size"].(int); ok { + scenario.valueSize = val } - if val, ok := params["num_operations"].(int); ok { - scenario.numOperations = val + if val, ok := params["num_operations"].(float64); ok { + scenario.numOperations = int(val) + } else if val, ok := params["num_operations"].(int); ok { + scenario.numOperations = val }tests/go/kvtest/pkg/scenarios/storage/memtable_stress.go-39-50 (1)
39-50: Type assertions forintwill fail with YAML-parsed parameters.When YAML is unmarshaled into
map[string]interface{}, numeric values are typicallyfloat64, notint. These type assertions will silently fail, leaving default values.🔎 Proposed fix
// Apply parameters from config - if val, ok := params["record_size"].(int); ok { - scenario.recordSize = val + if val, ok := params["record_size"].(float64); ok { + scenario.recordSize = int(val) + } else if val, ok := params["record_size"].(int); ok { + scenario.recordSize = val } - if val, ok := params["record_count"].(int); ok { - scenario.recordCount = val + if val, ok := params["record_count"].(float64); ok { + scenario.recordCount = int(val) + } else if val, ok := params["record_count"].(int); ok { + scenario.recordCount = val } - if val, ok := params["concurrent_writes"].(int); ok { - scenario.concurrentWriters = val + if val, ok := params["concurrent_writes"].(float64); ok { + scenario.concurrentWriters = int(val) + } else if val, ok := params["concurrent_writes"].(int); ok { + scenario.concurrentWriters = val } - if val, ok := params["flush_threshold"].(int); ok { - scenario.flushThreshold = val + if val, ok := params["flush_threshold"].(float64); ok { + scenario.flushThreshold = int(val) + } else if val, ok := params["flush_threshold"].(int); ok { + scenario.flushThreshold = val }tests/go/kvtest/pkg/scenarios/readyourwrites.go-34-36 (1)
34-36: Type assertion fortime.Durationwill likely fail.YAML typically unmarshals durations as strings (e.g.,
"100ms") or numbers. A directtime.Durationassertion won't match. Consider parsing from string or handling numeric values.🔎 Proposed fix
- if val, ok := params["delay"].(time.Duration); ok { - scenario.delay = val + if val, ok := params["delay"].(string); ok { + if d, err := time.ParseDuration(val); err == nil { + scenario.delay = d + } + } else if val, ok := params["delay"].(time.Duration); ok { + scenario.delay = val }tests/go/kvtest/pkg/core/integration.go-35-65 (1)
35-65: Context not cancelled after scenario execution.
NewTestContextcreates a cancellable context, butctx.Cancel()is never called after scenario execution. This could lead to goroutine/resource leaks if the context is passed to background operations.🔎 Proposed fix
for _, scenario := range e.scenarios { log.Printf("Running scenario: %s", scenario.Name()) ctx := NewTestContext(e.kvstore, e.config) + defer ctx.Cancel() ctx.TestPhase = "integration"Or use
deferat the start of the loop iteration scope by wrapping in a function:for _, scenario := range e.scenarios { + func() { log.Printf("Running scenario: %s", scenario.Name()) ctx := NewTestContext(e.kvstore, e.config) + defer ctx.Cancel() ctx.TestPhase = "integration" // ... rest of loop body + }() }conan/profiles/debug-clang-2-3 (1)
2-3: Compiler version mismatch: Profile uses non-versioned clang binaries but specifies compiler.version=20.The profile sets
CC=/usr/bin/clangandCXX=/usr/bin/clang++(lines 2-3) while explicitly requiringcompiler.version=20(line 10). This creates an inconsistency: the non-versioned binaries may not point to clang 20, and different environments may have different default clang versions, causing builds to silently use a mismatched compiler.Revert to explicit versioned paths:
[buildenv] -CC=/usr/bin/clang -CXX=/usr/bin/clang++ +CC=/usr/bin/clang-20 +CXX=/usr/bin/clang++-20src/include/concurrency/thread_safe_queue.h-186-189 (1)
186-189: Usedebuglog level instead ofinfofor empty queue diagnostics.The message "MEMTABLES ARE EMPTY" appears to be diagnostic/debug output. Using
infolevel will clutter production logs.🔎 Proposed fix
if (m_queue.empty()) { - spdlog::info("MEMTABLES ARE EMPTY"); + spdlog::debug("thread_safe_queue: Queue is empty during find"); }src/include/serialization/endian_integer.h-51-56 (1)
51-56: Missing bounds check infrom_bytes()can cause buffer overread.
from_bytes()copiesbytes.size()bytes without verifying that it equalssizeof(T). If the input span is smaller than expected, this causes undefined behavior; if larger, it only copies what fits but the caller may have intended different semantics.🔎 Proposed fix
static auto from_bytes(std::span<const std::byte> bytes) -> endian_integer { + if (bytes.size() != sizeof(T)) + { + // Consider throwing or returning std::optional<endian_integer> + throw std::invalid_argument("bytes span size does not match type size"); + } endian_integer result{0}; - std::memcpy(result.m_bytes.data(), bytes.data(), bytes.size()); + std::memcpy(result.m_bytes.data(), bytes.data(), sizeof(T)); return result; }Alternatively, use
SIZEconstant for the copy:- std::memcpy(result.m_bytes.data(), bytes.data(), bytes.size()); + static_assert(sizeof(T) == SIZE); + std::memcpy(result.m_bytes.data(), bytes.data(), SIZE);src/serialization/endian_integer_test.cpp-9-16 (1)
9-16: Test helperto_bytesproduces platform-dependent byte order.The
to_byteshelper usesstd::memcpy, which preserves the native byte order of the platform. However,endian_integer<T>defaults tostd::endian::little(as seen in Line 12 of endian_integer.h). On big-endian systems, theFromBytestest at Line 56 will fail becauseto_bytesproduces big-endian bytes whileendian_integerproduces little-endian bytes.🔎 Proposed fix to make to_bytes match endian_integer's byte order
template <typename T> requires std::is_integral_v<T> auto to_bytes(T value) noexcept -> std::array<std::byte, sizeof(T)> { + // Match endian_integer's default little-endian byte order + if constexpr (std::endian::native != std::endian::little) + { + value = std::byteswap(value); + } std::array<std::byte, sizeof(T)> bytes; std::memcpy(bytes.data(), &value, sizeof(T)); return bytes; }src/structures/lsmtree/lsmtree_test_basic.cpp-215-256 (1)
215-256: Timing-dependent test relies on fixed sleep duration due to lack of public flush completion API.Line 239 uses
std::this_thread::sleep_for(std::chrono::seconds(1))to wait for background flush operations to complete. While internal synchronization mechanisms exist (e.g.,absl::Notificationinlevels_t), they are not exposed through the public LSMTree API. This forces the test to rely on a fixed sleep duration, which can be flaky under load.Consider adding a public API method to LSMTree that allows tests to wait deterministically for flush completion (e.g.,
waitForFlush()with timeout, or a status query likeisPendingFlush()), similar to how the internal compaction thread usesabsl::Notification::WaitForNotificationWithTimeout().src/structures/lsmtree/segments/lsmtree_regular_segment.cpp-129-133 (1)
129-133: Timestamp truncated fromuint64_ttouint32_t.
record.m_timestamp.m_value.time_since_epoch().count()returns a 64-bit value, but it's cast touint32_tbefore being stored in ale_uint64_t. This loses the upper 32 bits.Proposed fix
.write_endian_integer( - serialization::le_uint64_t{static_cast<std::uint32_t>( + serialization::le_uint64_t{static_cast<std::uint64_t>( record.m_timestamp.m_value.time_since_epoch().count() )} )src/structures/lsmtree/segments/lsmtree_regular_segment.cpp-447-456 (1)
447-456:min()andmax()dereferencem_memtablewithout checkinghas_value().If
m_memtableisstd::nullopt, callingm_memtable->min()orm_memtable->max()is undefined behavior.Proposed fix
auto regular_segment_t::min() const noexcept -> std::optional<record_t::key_t> { + if (!m_memtable.has_value()) + { + return std::nullopt; + } return m_memtable->min(); } std::optional<record_t::key_t> regular_segment_t::max() const noexcept { + if (!m_memtable.has_value()) + { + return std::nullopt; + } return m_memtable->max(); }src/structures/lsmtree/segments/lsmtree_regular_segment.cpp-346-346 (1)
346-346: Potential buffer overrun ifoffset >= fileSize.If
offsetequals or exceedsfileSize, the span size calculationfileSize - offsetwill underflow (wrap to a huge value) or be zero/negative, causing undefined behavior when reading.Proposed fix
+ if (offset >= fileSize) + { + spdlog::error( + "Invalid offset {} exceeds file size {}. path={}", + offset, fileSize, get_path().string() + ); + munmap(mmapPtr, fileSize); + return std::nullopt; + } + std::span<std::byte> mmapSpan{static_cast<std::byte *>(mmapPtr) + offset, fileSize - offset};src/include/common/helpers.h-20-42 (1)
20-42: RNG created on every call defeats performance benefits of mt19937.Creating a new
std::mt19937and seeding fromstd::random_deviceon every call is expensive. Usestatic thread_localto reuse the generator.Additionally, the
else ifbranches after line 24 should useelse if constexprto avoid instantiation errors for unsupported types, and returning0for unsupported types is a silent failure.Proposed fix
template <typename TNumber> auto generateRandomNumber( const TNumber min = std::numeric_limits<TNumber>::min(), const TNumber max = std::numeric_limits<TNumber>::max() ) noexcept -> TNumber { - std::mt19937 rng{std::random_device{}()}; + static thread_local std::mt19937 rng{std::random_device{}()}; if constexpr (std::is_same_v<int, TNumber>) { return std::uniform_int_distribution<TNumber>(min, max)(rng); } - else if (std::is_same_v<std::size_t, TNumber>) + else if constexpr (std::is_same_v<std::size_t, TNumber>) { return std::uniform_int_distribution<TNumber>(min, max)(rng); } - else if (std::is_same_v<double, TNumber>) + else if constexpr (std::is_same_v<double, TNumber>) { return std::uniform_real_distribution<double>(min, max)(rng); } - else if (std::is_same_v<float, TNumber>) + else if constexpr (std::is_same_v<float, TNumber>) { return std::uniform_real_distribution<float>(min, max)(rng); } else { - // TODO(vahag): better handle this case - return 0; + static_assert(sizeof(TNumber) == 0, "Unsupported type for generateRandomNumber"); } }src/structures/lsmtree/lsmtree.cpp-180-183 (1)
180-183: Flush failures are silently ignored.The
assert(ok)is commented out, meaning ifflush_to_level0fails, the error is silently ignored. This could lead to data loss. The stop path at line 159 still has the assert, creating inconsistent error handling.🔎 Proposed fix
absl::WriterMutexLock lock{&m_mutex}; bool ok{m_pLevels->flush_to_level0(std::move(memtable))}; - // assert(ok); + if (!ok) + { + spdlog::error("Failed to flush memtable to level0"); + // Consider implementing retry logic or error propagation + }
🟡 Minor comments (18)
tests/go/kvtest/pkg/validators/interface.go-1-3 (1)
1-3: Remove this empty, unused placeholder file.The
validatorspackage in this file is not imported anywhere in the codebase. Since the file contains no definitions and serves no purpose, remove it entirely.tests/go/kvtest/pkg/scenarios/factory.go-26-26 (1)
26-26: Fix typo in error message.The error message contains a typo: "unkown" should be "unknown".
🔎 Proposed fix
default: - return nil, fmt.Errorf("unkown scenario: %s", config.Type) + return nil, fmt.Errorf("unknown scenario: %s", config.Type) }tests/go/kvtest/pkg/core/config.go-45-45 (1)
45-45: Typo in YAML tag:durartionshould beduration.This typo will prevent proper YAML deserialization of the
Durationfield.🔎 Proposed fix
- Duration time.Duration `yaml:"durartion"` + Duration time.Duration `yaml:"duration"`src/structures/skiplist/skiplist.cpp-1-1 (1)
1-1: This file is unnecessary—it contains only a header include and serves no purpose.The SkipList library target is built from skiplist.cpp but is only linked to the test executable. Since skiplist.cpp provides no implementation (only
#include "structures/skiplist/skiplist.h"), the entire file and its corresponding CMakeLists.txt entry can be removed. The test can link directly to the header library or the header dependency instead.src/structures/skiplist/skiplist_test.cpp-1-18 (1)
1-18: Include the correct header for skiplist.Line 5 includes
structures/lsmtree/lsmtree.h, but the tests useskiplist::skiplist_t. This is likely incorrect. Please includestructures/skiplist/skiplist.hdirectly instead of relying on transitive includes through lsmtree.🔎 Proposed fix
#include <gtest/gtest.h> #include <algorithm> -#include "structures/lsmtree/lsmtree.h" +#include "structures/skiplist/skiplist.h"src/serialization/CMakeLists.txt-61-62 (1)
61-62: Wrong target name inset_target_properties.Line 62 sets
CXX_STANDARD 23onVarintTestinstead ofCRC32Test. This meansCRC32Testwon't have the C++ standard explicitly set, andVarintTestis set twice.🔎 Proposed fix
# CRC32 add_executable(CRC32Test crc32_test.cpp) -set_target_properties(VarintTest PROPERTIES CXX_STANDARD 23) +set_target_properties(CRC32Test PROPERTIES CXX_STANDARD 23)src/serialization/buffer_reader_writer_test.cpp-33-39 (1)
33-39:FromBytestest does not exercisefrom_bytes().This test is identical to the
Gettest and doesn't actually callle_int8_t::from_bytes(). It should construct anendian_integerfrom a byte span and verify the result.🔎 Proposed fix to actually test `from_bytes()`
TEST(EndianInteger, FromBytes) { const std::int8_t podInt8{12}; const le_int8_t expectedInt8{podInt8}; + + // Actually test from_bytes + const auto reconstructed = le_int8_t::from_bytes(expectedInt8.bytes()); - EXPECT_EQ(expectedInt8.get(), podInt8); + EXPECT_EQ(reconstructed.get(), podInt8); }src/common/helpers.cpp-34-51 (1)
34-51: Inconsistent byte ranges:disexcludes 0 butdis8includes 0.The distribution
dis(1, 255)excludes null bytes for 8-byte chunks, but the default-constructeddis8produces values in[0, 255], potentially including null bytes for remaining bytes. This inconsistency may cause issues if null bytes in strings are problematic.🔎 Suggested fix for consistency
- std::uniform_int_distribution<uint8_t> dis8; + std::uniform_int_distribution<uint8_t> dis8(1, 255);src/structures/lsmtree/levels/levels.cpp-23-23 (1)
23-23: Redundant doublestd::moveon pManifest.Same issue as in
level.cpp- the nestedstd::moveis unnecessary.Proposed fix
- m_pManifest{std::move(std::move(pManifest))}, + m_pManifest{std::move(pManifest)},src/raft/raft.cpp-486-488 (1)
486-488:spdlog::errorformat string mismatch.Same issue - format string missing second placeholder.
Proposed fix
- spdlog::error( - "Node={} is unable to persist votedFor", m_nodeConfig.m_id, m_votedFor - ); + spdlog::error( + "Node={} is unable to persist votedFor={}", m_nodeConfig.m_id, m_votedFor + );src/raft/raft.cpp-423-423 (1)
423-423:spdlog::errorformat string mismatch.Format string has one placeholder but two arguments are provided.
Proposed fix
- spdlog::error("Node={} is unable to persist votedFor", m_nodeConfig.m_id, m_votedFor); + spdlog::error("Node={} is unable to persist votedFor={}", m_nodeConfig.m_id, m_votedFor);src/structures/lsmtree/levels/levels.cpp-226-232 (1)
226-232: Silently ignoring compaction failures may mask issues.The commented-out error log and lack of any failure handling means compaction errors will go unnoticed. Consider at least logging at debug level or tracking failure metrics.
Proposed fix
auto ok{compact()}; if (!ok) { - // spdlog::error("Compaction failed."); + spdlog::debug("Compaction skipped or not required at this time"); } - // TODO(lnikon): compact returns null to indicate that compaction didn't happened or on - // error, which causes false-positives on this ASSERT ASSERT(ok);src/fs/random_access_file.cpp-140-153 (1)
140-153:errnois not meaningful whenm_fd == -1check fails.When
m_fd == -1, no system call failed - this is just a state check. Capturingerrnohere is misleading since it reflects some previous, unrelated syscall (or is undefined). Consider settingsystem_errnoto 0 or removing it from this error path.Proposed fix
if (m_fd == -1) { return std::unexpected( file_error_t{ .code = file_error_code_k::invalid_file_descriptor, - .system_errno = errno, + .system_errno = 0, .message = fmt::format("Invalid file descriptor. fd={}", m_fd), } ); }src/fs/random_access_file.cpp-261-270 (1)
261-270: Empty path check captures staleerrno.When
path.empty()is true, no system call has failed, soerrnoandstrerror(errno)are meaningless or reflect a previous unrelated call.Proposed fix
if (path.empty()) { return std::unexpected( file_error_t{ .code = file_error_code_k::open_failed, - .system_errno = errno, - .message = fmt::format("Provided file path is empty. errno={}", strerror(errno)), + .system_errno = 0, + .message = "Provided file path is empty", } ); }src/raft/raft.cpp-403-405 (1)
403-405:spdlog::errorformat string mismatch - two placeholders, one argument.The format string contains two
{}placeholders but onlym_nodeConfig.m_idandm_commitIndexare provided, while the message mentions "commitIndex" twice.Proposed fix
- spdlog::error( - "Node={} is unable to persist commitIndex", m_nodeConfig.m_id, m_commitIndex - ); + spdlog::error( + "Node={} is unable to persist commitIndex={}", m_nodeConfig.m_id, m_commitIndex + );src/include/serialization/buffer_reader.h-90-90 (1)
90-90: Potential truncation on 32-bit platforms.
countisstd::uint64_tbutread_raw_bytestakesstd::size_t. On 32-bit systems, this could truncate large values. Consider adding a bounds check or usingstd::size_tforcount.src/include/common/uuid.h-874-880 (1)
874-880: Thread-safety issue inget_clock_sequence().The static
std::mt19937 clock_genandstd::uniform_int_distribution clock_disare not thread-safe. Concurrent calls can corrupt RNG state. Whileclock_sequenceis atomic, the initialization race onclock_dis(clock_gen)is problematic.Since this is behind
UUID_TIME_GENERATORand has a "DO NOT USE IN PRODUCTION" warning, this may be acceptable, but consider adding a mutex if this code path is used.src/include/common/uuid.h-29-33 (1)
29-33: Remove unreachable GSL fallback—project requires C++23.The project's CMakeLists.txt sets
CMAKE_CXX_STANDARD 23, which is well beyond the C++20 threshold in the conditional check. This meansLIBUUID_CPP20_OR_GREATERis always true and the#elsebranch (lines 31–32) is unreachable dead code.Remove the fallback:
#ifdef LIBUUID_CPP20_OR_GREATER #include <span> -#else -#include <gsl/span> #endifNote: While GSL is removed from this header, the project still lists
Microsoft.GSLas a required dependency in the main CMakeLists.txt (used elsewhere), so broader GSL removal would require additional changes.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
- Fixing issues found during code review
- Fixing issues found during code review
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
conan/profiles/debug-clang-tsan (1)
2-3: Version mismatch between compiler binary and declared version.The
CC/CXXpaths referenceclang-19, butcompiler.versiondeclares21. Conan will generate flags for clang 21 while actually using clang 19, potentially causing ABI issues or build failures.🔎 Proposed fix
Either update the binary paths to match version 21:
[buildenv] -CC=/usr/bin/clang-19 -CXX=/usr/bin/clang++-19 +CC=/usr/bin/clang-21 +CXX=/usr/bin/clang++-21 CXXFLAGS=-fsanitize=thread -g -O1Or use unversioned paths like
debug-clang:[buildenv] -CC=/usr/bin/clang-19 -CXX=/usr/bin/clang++-19 +CC=/usr/bin/clang +CXX=/usr/bin/clang++ CXXFLAGS=-fsanitize=thread -g -O1Also applies to: 13-13
conan/profiles/release-clang (1)
2-3: Version mismatch between compiler binary and declared version.Same issue as
debug-clang-tsan:CC/CXXpaths referenceclang-20, butcompiler.versiondeclares21.🔎 Proposed fix
Update paths to match version 21:
[buildenv] -CC=/usr/bin/clang-20 -CXX=/usr/bin/clang++-20 +CC=/usr/bin/clang-21 +CXX=/usr/bin/clang++-21Or use unversioned paths for consistency with
debug-clang:[buildenv] -CC=/usr/bin/clang-20 -CXX=/usr/bin/clang++-20 +CC=/usr/bin/clang +CXX=/usr/bin/clang++Also applies to: 10-10
src/include/structures/skiplist/skiplist.h (1)
157-195: UsenewLevelinstead ofm_levelfor node construction.On line 182, the new node should be constructed with
newLevel(the node's assigned random level), notm_level(the skiplist's current maximum level). In standard skiplist implementations, each node is allocated with capacity for its own random level, not the global maximum. UsingnewLevelensures proper memory efficiency and correct skiplist semantics.- auto newNode = std::make_shared<node_t>(std::forward<TRecord>(record), m_level); + auto newNode = std::make_shared<node_t>(std::forward<TRecord>(record), newLevel);src/structures/lsmtree/lsmtree.cpp (3)
381-394: Return nullopt when level recreation fails.Lines 381-385 detect when level recreation fails and break the loop, but line 394 still returns the partially constructed
pLevelsobject. This could result in using corrupted or incomplete LSM tree state, potentially causing data loss or crashes. The function should returnstd::nulloptwhenfailedToRecreateLevelis true.🔎 Proposed fix to halt on unrecoverable errors
if (failedToRecreateLevel) { spdlog::error("Failed to recreate level {}", lastLevelIndex); break; } } + if (failedToRecreateLevel) + { + spdlog::error("Cannot proceed with incomplete level structure"); + return std::nullopt; + } + spdlog::debug("Restoring levels"); pLevels->restore(); spdlog::debug("Recovery finished"); pManifest->enable(); return std::make_optional<std::unique_ptr<levels::levels_t>>(std::move(pLevels));
55-74: Address the unused error variant in theput()method signature.The method returns
std::expected<lsmtree_success_k, lsmtree_error_k>but never returns the error variant—all code paths return success variants (ok_kormemtable_reset_k). Additionally,memtable_t::emplace()returnsvoidand cannot fail gracefully. Either:
- Implement proper error handling (e.g., catch allocation failures in memtable operations and return
lsmtree_error_k::put_failed_k), or- Change the return type to
lsmtree_success_kand remove the unused error variant if no error cases are planned.The existing TODO comment at line 62 ("Can memtable emplace fail?") indicates this gap is already known.
221-253: Add error handling for protobuf parsing in WAL recovery.Line 224 calls
ParseFromStringwithout checking if parsing succeeded. The protobuf library'sParseFromStringmethod returnsbool—trueon successful parsing,falseon invalid or corrupted data. If parsing fails,dbOperationwill contain default values, causing incorrect recovery behavior or silent data corruption.All four
ParseFromStringcalls in the codebase lack error checking, making this a systemic issue. Add validation before using the parsed operation:Proposed fix
for (const auto &walEntries{pWal->records()}; const raft::v1::LogEntry &walEntry : walEntries) { tinykvpp::v1::DatabaseOperation dbOperation; - dbOperation.ParseFromString(walEntry.payload()); + if (!dbOperation.ParseFromString(walEntry.payload())) + { + spdlog::error("Failed to parse DatabaseOperation from WAL entry"); + continue; + } switch (dbOperation.type())
♻️ Duplicate comments (1)
src/structures/lsmtree/segments/lsmtree_regular_segment.cpp (1)
340-383: Memory leak:mmapresult is nevermunmap'd.The mmap at line 340 allocates memory that is never released before returning. This was flagged in a previous review.
🟡 Minor comments (17)
src/server/grpc_server.cpp-136-139 (1)
136-139: Fix typos in log message.Line 138 contains two typos: "Excetion" should be "Exception" and "occured" should be "occurred".
The change to use
const std::exception&is an appropriate improvement.🔎 Proposed fix
- spdlog::error("Excetion occured while shutting down gRPC server. {}", e.what()); + spdlog::error("Exception occurred while shutting down gRPC server. {}", e.what());src/server/grpc_server.cpp-39-43 (1)
39-43: Fix typo in log message.Line 41 contains a typo: "Excetion" should be "Exception".
The changes to use
const std::exception&and returngrpc::INTERNALstatus are appropriate improvements.🔎 Proposed fix
- spdlog::error("Excetion in Put operation: {}", e.what()); + spdlog::error("Exception in Put operation: {}", e.what());src/server/grpc_server.cpp-61-65 (1)
61-65: Fix typo in log message.Line 63 contains a typo: "Excetion" should be "Exception".
The changes to use
const std::exception&and returngrpc::INTERNALstatus are appropriate improvements.🔎 Proposed fix
- spdlog::error("Excetion in Get operation: {}", e.what()); + spdlog::error("Exception in Get operation: {}", e.what());src/server/grpc_server.cpp-118-121 (1)
118-121: Fix typos in log message.Line 120 contains two typos: "Excetion" should be "Exception" and "occured" should be "occurred".
The change to use
const std::exception&is an appropriate improvement.🔎 Proposed fix
- spdlog::error("Excetion occured while creating gRPC server. {}", e.what()); + spdlog::error("Exception occurred while creating gRPC server. {}", e.what());proto/raft/v1/raft_service.proto-62-66 (1)
62-66: Test proto has not been updated to match the main proto.Using
bytesinstead ofstringforpayloadsin the main proto is appropriate for binary payload data and remains wire-compatible. However, the test proto attests/go/kvtest/proto/raft/v1/raft_service.protostill definesrepeated string payloads = 3;(line 64) and needs to be updated torepeated bytes payloads = 3;for consistency.Additionally, the test proto's
LogEntryis missing thereserved 3;field declaration present in the main proto.src/fs/random_access_file.cpp-286-297 (1)
286-297: io_uring failure may not set errno.
io_uring_queue_initreturns a negative error code directly (not via errno). Usingstrerror(errno)here may report an unrelated error. The actual error is in-res.Proposed fix
if (int res = io_uring_queue_init(kIOUringQueueEntries, &ring, 0); res < 0) { close(fdes); return std::unexpected( file_error_t{ .code = file_error_code_k::io_uring_init_failed, - .system_errno = errno, - .message = fmt::format("io_uring_queue_init failed. errno={}", strerror(errno)), + .system_errno = -res, + .message = fmt::format("io_uring_queue_init failed. errno={}", strerror(-res)), } ); }src/fs/random_access_file.cpp-261-270 (1)
261-270: Inconsistent errno usage when path is empty.When the path is empty, no system call has failed, so
errnomay contain a stale value from a previous unrelated operation. Thesystem_errnois correctly set to 0, but the message usesstrerror(errno)which is misleading.Proposed fix
if (path.empty()) { return std::unexpected( file_error_t{ .code = file_error_code_k::open_failed, .system_errno = 0, - .message = fmt::format("Provided file path is empty. errno={}", strerror(errno)), + .message = "Provided file path is empty", } ); }src/structures/lsmtree/segments/lsmtree_regular_segment.cpp-463-467 (1)
463-467: Potential null dereference in num_of_bytes_used().The
ASSERT(m_memtable.has_value())will terminate the program ifm_memtableis empty. Consider returning 0 or throwing an exception instead of asserting, especially sincemoved_memtable()can leavem_memtablein an empty state.Proposed fix
auto regular_segment_t::num_of_bytes_used() const -> std::size_t { - ASSERT(m_memtable.has_value()); - return m_hashIndex.num_of_bytes_used() + m_memtable->num_of_bytes_used(); + const auto memtableSize = m_memtable.has_value() ? m_memtable->num_of_bytes_used() : 0; + return m_hashIndex.num_of_bytes_used() + memtableSize; }src/structures/lsmtree/lsmtree_test_basic.cpp-241-255 (1)
241-255: Fix duplicate increment of loop counter.The variable
idxis incremented twice per iteration: once in the log statement (line 244) and again at line 254. This causes the counter to skip values and makes the log messages misleading.🔎 Proposed fix
std::size_t idx{0}; for (const auto &rec : records) { spdlog::info("Verifying record {}/{}", ++idx, recordCount); auto expected{pLsm->get(rec.m_key)}; EXPECT_TRUE(expected.has_value()); spdlog::info( "expected->m_key.m_key.size()={}, rec.m_key.m_key={}", expected->m_key.m_key.size(), rec.m_key.m_key.size() ); EXPECT_EQ(expected->m_key.m_key, rec.m_key.m_key); EXPECT_EQ(expected->m_value.m_value, rec.m_value.m_value); - ++idx; }src/structures/lsmtree/lsmtree_test_basic.cpp-67-74 (1)
67-74: Remove the duplicate operator>> definition from the test file.This operator>> is already defined in
src/include/raft/raft.h(lines 31-34) and duplicated here. Additionally, it is not used anywhere in this test file, making it dead code. Remove lines 67-74 entirely and rely on the header definition.Note: The implementation pattern (using
stream.str()) differs from other operators in the codebase (manifest.handwal_test.cpp), which use direct stream extraction. Consider whether the LogEntry operator should follow the same pattern for consistency.src/raft/raft.cpp-423-423 (1)
423-423: Fix the format string mismatch.The format string has only one placeholder
{}but two arguments are provided. The second argumentm_votedForwill be ignored.🔎 Proposed fix
- spdlog::error("Node={} is unable to persist votedFor", m_nodeConfig.m_id, m_votedFor); + spdlog::error("Node={} is unable to persist votedFor={}", m_nodeConfig.m_id, m_votedFor);src/raft/raft.cpp-403-405 (1)
403-405: Fix the format string mismatch.The format string has only one placeholder
{}but two arguments are provided. The second argumentm_commitIndexwill be ignored.🔎 Proposed fix
- spdlog::error( - "Node={} is unable to persist commitIndex", m_nodeConfig.m_id, m_commitIndex - ); + spdlog::error( + "Node={} is unable to persist commitIndex={}", m_nodeConfig.m_id, m_commitIndex + );src/include/concurrency/thread_safe_queue.h-13-13 (1)
13-13: Remove the unuseddb/db_config.hinclude.The header includes
db/db_config.hbut does not use any symbols from it. Removing this include reduces unnecessary compilation coupling and dependencies.src/include/concurrency/thread_safe_queue.h-39-47 (1)
39-47: Clarify documentation and design rationale for the transactional interface.The comment on line 42 states the method returns items in "oldest-to-newest order (queue in a reversed order)," but reversing a deque (where front=oldest) yields newest-to-oldest. Please verify and correct the documentation.
Additionally, the reserve/consume API would benefit from:
- Explanation of the use case and typical usage pattern
- Guarantees about concurrent operations (can
push()occur betweenreserve()andconsume()?)- Behavior if
consume()is never called afterreserve()- Whether a reservation times out or can be canceled
Suggested documentation improvements
// Transactional interface. + // Use case: Atomically reserve all queued items for processing, then consume them once confirmed. + // Note: push() may still add items after reserve(); consume() only removes reserved items. // NOLINTBEGIN(modernize-use-trailing-return-type) /** - * Returns stored items in oldest-to-newest order (queue in a reversed order). + * Reserves all current items and returns them in newest-to-oldest order (reversed queue). + * Returns nullopt if queue is empty or a reservation already exists. + * Must call consume() to remove reserved items from the queue. */ std::optional<std::vector<TItem>> reserve() noexcept ABSL_LOCKS_EXCLUDED(m_mutex); + /** + * Consumes items up to the reservation point set by reserve(). + * Returns true on success, false if no reservation exists. + */ bool consume() noexcept ABSL_LOCKS_EXCLUDED(m_mutex);src/include/serialization/buffer_reader.h-52-70 (1)
52-70: Validate the 10th byte value to prevent overflow.When processing the 10th varint byte (at shift=63, line 60), the code doesn't validate that only bit 0 is set. A properly encoded 64-bit varint should ensure byte 9 contains at most the value 1. If a malformed varint has byte 9 with value 0x7F (127), the expression
(0x7F << 63)will overflow the uint64_t, producing incorrect results.Consider adding validation before the shift on byte 9:
🔎 Proposed fix
while (byteCount < MAX_VARINT_BYTES) { const auto bytes{read_raw_bytes(1)}; if (has_error()) { return *this; } + + // On the 10th byte, only bit 0 should be set + if (byteCount == 9 && (bytes[0] & std::byte{0x7E}) != std::byte{0}) + { + m_error = serialization_error_k::invalid_varint_k; + return *this; + } result |= (std::to_integer<std::uint64_t>((bytes[0] & std::byte{0x7F})) << shift);src/include/serialization/buffer_reader.h-18-21 (1)
18-21: Change buffer_reader_t to use const spans for true read-only semantics.The constructor and internal buffer should accept and store
std::span<const std::byte>instead ofstd::span<std::byte>. The class only reads from the buffer—all public methods parse data into output parameters without modifying the buffer itself. Using mutable spans unnecessarily allows callers to mutate the buffer through returned spans, violating const-correctness and increasing the risk of subtle bugs in concurrent or shared-buffer scenarios.src/structures/lsmtree/levels/levels.cpp-219-234 (1)
219-234: Improve error handling and remove commented code.The compaction task loop has several issues:
- Commented-out error log (line 229): Either remove it or uncomment it to handle errors properly.
- Ambiguous null return: The TODO comment (lines 231-232) notes that
compact()returns null for both "compaction didn't happen" and "error" cases, causing false positives. This design makes error handling unclear.Consider one of these approaches:
🔎 Option 1: Use std::optional to distinguish "no compaction" from "error"
Update
compact()to returnstd::optional<shared_ptr_t>where:
std::nullopt= no compaction needed (not an error)std::optionalwith null pointer = errorstd::optionalwith valid pointer = successThen handle explicitly:
auto result = compact(); if (!result.has_value()) { // No compaction needed, not an error } else if (!result.value()) { spdlog::error("Compaction failed."); } else { // Compaction succeeded }🔎 Option 2: Return a status or result type
enum class CompactionResult { Success, NotNeeded, Error }; struct CompactionOutcome { CompactionResult status; segments::regular_segment::shared_ptr_t segment; };This makes the three states explicit.
🧹 Nitpick comments (35)
docs/sstable_binary_format_spec_v1.md (2)
57-57: Clarify the semantics of thetimestampfield.Line 57 introduces a
u32 timestampin the Body section without explaining its purpose. Is this a write timestamp, last-access time, or something else? This should be documented to avoid implementation ambiguity.
9-44: Specify byte order (endianness) for multi-byte fields.The spec defines u32 and u64 fields (magic, version, body_offset, index_offset, payload_size, checksum, seq_num, timestamp, body_off) but does not state whether they are serialized as big-endian or little-endian. This is critical for cross-platform compatibility. Add a section explicitly stating endianness (e.g., "All multi-byte integers are stored in little-endian byte order").
src/include/fs/mmap_random_access_file.h (1)
26-30: Consider using unsigned types for offsets.The
writemethod usesssize_t offset(signed) whilereadalso usesssize_t offset. File offsets are typically non-negative. Usingsize_toroff_twould be more consistent with POSIX conventions and prevent accidental negative offset values.🔎 Proposed fix
- [[nodiscard]] auto write(std::string_view data, ssize_t offset) noexcept + [[nodiscard]] auto write(std::string_view data, off_t offset) noexcept -> std::expected<ssize_t, file_error_t>; - [[nodiscard]] auto read(ssize_t offset, char *buffer, size_t size) noexcept + [[nodiscard]] auto read(off_t offset, char *buffer, size_t size) noexcept -> std::expected<std::size_t, file_error_t>;Dockerfile (1)
37-64: Consider removing commented-out code.Large blocks of commented-out code add noise and can become stale. If the multi-stage build is no longer needed, remove it entirely. If it's temporarily disabled for debugging or future use, consider adding a brief comment explaining the intent or tracking removal in an issue.
proto/raft/v1/raft_service.proto (1)
24-29: Consider reserving the field name alongside the field number.When reserving a removed field, protobuf best practice is to reserve both the field number and the field name to prevent accidental reuse of either:
reserved 3; reserved "old_field_name";This provides better documentation and prevents future developers from accidentally reusing the field name with a different number.
tests/go/kvtest/pkg/scenarios/readyourwrites.go (1)
57-57: Consider more readable error formatting for byte values.The current format with
%vprints byte slices as numeric arrays, which can be hard to read during debugging. Consider using%sfor string representation or%xfor hex encoding to improve readability of test failures.💡 Example alternatives
-return fmt.Errorf("data consistency error: expected %v, got %v", expectedValue, value) +return fmt.Errorf("data consistency error: expected %q, got %q", expectedValue, value)Or for hex representation:
-return fmt.Errorf("data consistency error: expected %v, got %v", expectedValue, value) +return fmt.Errorf("data consistency error: expected %x, got %x", expectedValue, value)src/include/structures/memtable/memtable.h (5)
22-36: Consider adding noexcept to comparison operators.The comparison operators (
operator<,operator>,operator==) compare strings and could be markednoexceptsince string comparison operations don't throw in this context.🔎 Suggested noexcept additions
- auto operator<(const key_t &other) const -> bool; - auto operator>(const key_t &other) const -> bool; - auto operator==(const key_t &other) const -> bool; + auto operator<(const key_t &other) const noexcept -> bool; + auto operator>(const key_t &other) const noexcept -> bool; + auto operator==(const key_t &other) const noexcept -> bool;
67-68: TODO: Address the default constructor concern or document why it's needed.The TODO comment indicates uncertainty about whether the default constructor should exist. Consider either:
- Removing the default constructor if records should always be fully initialized
- Documenting why it's needed (e.g., for container compatibility)
- Making it
= deleteif it shouldn't be usedIf you'd like to track this as a separate task, I can help create an issue to address this design decision.
76-78: Add noexcept to comparison operators if appropriate.Similar to the
key_toperators, these record comparison operators could be markednoexceptfor better performance and clearer exception guarantees.🔎 Suggested noexcept additions
- auto operator<(const record_t &record) const -> bool; - auto operator>(const record_t &record) const -> bool; - auto operator==(const record_t &record) const -> bool; + auto operator<(const record_t &record) const noexcept -> bool; + auto operator>(const record_t &record) const noexcept -> bool; + auto operator==(const record_t &record) const noexcept -> bool;
96-98: Remove commented-out code.The commented-out
sorted_vectoralternative should either be removed or, if it represents a viable alternative implementation, moved to documentation or design notes. Commented code in production headers reduces readability.🔎 Proposed cleanup
- // using storage_t = typename sorted_vector::sorted_vector_t<record_t, - // record_comparator_by_key_t>; using storage_t = typename skiplist::skiplist_t<record_t, record_comparator_by_key_t>;If this represents an important design consideration, document it in a separate design doc or ADR (Architecture Decision Record).
105-105: Consider taking record_t by const reference or forwarding reference.The
emplacemethod takesrecord_tby value, which means the caller must always move or copy. Consider using a forwarding reference or const reference depending on the intended semantics:
void emplace(const record_t &record)if you need to keep the originalvoid emplace(record_t &&record)if you only accept rvalues- Current signature requires the caller to explicitly move
The current signature forces a move at the call site, which is explicit but may be less flexible. If you want to accept both lvalues and rvalues efficiently, consider:
void emplace(record_t &&record); // Accept only rvalues (explicit move required)or for maximum flexibility:
template<typename T> void emplace(T&& record); // Perfect forwardingsrc/structures/lsmtree/lsmtree_test_basic.cpp (4)
123-135: Remove unused variable.Line 125 declares
randomKeysbut never uses it. This appears to be leftover from copying the test template.🔎 Proposed fix
TEST(LSMTreeTestBasicCRUD, GetNonExistentKey) { - const auto &randomKeys = common::generateRandomStringPairVector(16_B); auto pConfig{generateConfig(16_B, 64_B, 128_B)}; auto pLsm{generateLSMTree(pConfig)};
137-163: Remove unused variable.Line 139 declares
randomKeysbut never uses it, similar to the previous test.🔎 Proposed fix
TEST(LSMTreeTestBasicCRUD, UpdateExistingKey) { - const auto &randomKeys = common::generateRandomStringPairVector(16_B); auto pConfig{generateConfig(16_B, 64_B, 128_B)}; auto pLsm{generateLSMTree(pConfig)};
165-177: Remove unused variable.Line 167 declares
randomKeysbut never uses it, same pattern as the previous tests.🔎 Proposed fix
TEST(LSMTreeTestBasicCRUD, EmptyTreeGet) { - const auto &randomKeys = common::generateRandomStringPairVector(16_B); auto pConfig{generateConfig(16_B, 64_B, 128_B)}; auto pLsm{generateLSMTree(pConfig)};
239-239: Replace fixed sleep with proper synchronization.The hardcoded 1-second sleep suggests the test depends on asynchronous flush completion. This creates timing dependencies that make tests slower and potentially flaky if the flush takes longer under load.
Consider replacing with:
- An explicit flush/sync operation if available on the LSMTree API
- A polling mechanism with timeout
- Event-based synchronization if the LSMTree supports flush completion callbacks
src/include/serialization/endian_integer.h (2)
16-19: Consider constraining template parameterTto integral types.The template accepts any type
T, but endianness conversion is primarily meaningful for integer types. Usingendian_integerwith floating-point types or pointers could lead to subtle bugs or misuse.🔎 Proposed constraint using concepts
-template <typename T, std::endian Target = std::endian::little> class endian_integer +template <std::integral T, std::endian Target = std::endian::little> +class endian_integer { public: static constexpr size_t SIZE = sizeof(T);Alternatively, use a static assertion inside the class:
template <typename T, std::endian Target = std::endian::little> class endian_integer { public: + static_assert(std::is_integral_v<T>, "endian_integer requires an integral type"); static constexpr size_t SIZE = sizeof(T);
51-58: Optional: Optimizefrom_bytesto avoid unnecessary initialization.The method creates
endian_integer result{0}, which initializesm_bytesvia the constructor, then immediately overwrites those bytes. While functionally correct, this is slightly inefficient.🔎 Proposed optimization
Add a private default constructor and use it in
from_bytes:static auto from_bytes(std::span<const std::byte> bytes) -> endian_integer { assert(bytes.size() == sizeof(T)); - endian_integer result{0}; + endian_integer result; // calls private default ctor std::memcpy(result.m_bytes.data(), bytes.data(), sizeof(T)); return result; } private: + endian_integer() = default; // private default constructor std::array<std::byte, sizeof(T)> m_bytes; };src/include/concurrency/thread_safe_queue.h (3)
29-30: LGTM: Move semantics appropriately deleted.Deleting move operations is correct since
absl::Mutexandstd::atomic<bool>are not movable. Consider adding a brief comment explaining this constraint for future maintainers.Optional: Add explanatory comment
+ // Move operations deleted because mutex and atomic members are not movable thread_safe_queue_t(thread_safe_queue_t &&other) noexcept = delete; auto operator=(thread_safe_queue_t &&other) noexcept -> thread_safe_queue_t & = delete;
107-126: Consider memory efficiency for large queues.The
reserve()method creates a full copy of the queue in reversed order (line 125), which requires O(n) time and space. For large queues, this could impact performance.Consider these alternatives:
- Return a view or iterator pair instead of copying
- Process items in-place without materializing a copy
- Document the memory cost if the current design is intentional
Additionally, verify that the "reversed order" is the desired behavior for your use case.
Alternative: Return const reference with iterator guidance
If callers can process items using iterators, you could return a read-only reference:
// Return pair of iterators (oldest to newest) std::optional<std::pair<typename queue_t::const_iterator, typename queue_t::const_iterator>> reserve() noexcept ABSL_LOCKS_EXCLUDED(m_mutex);However, this complicates lifetime management. The copy is safer but more expensive.
103-103: Reconsider logging level forpop_all().The
infolevel log on line 103 might be too verbose for production. Consider:
- Using
debuglevel (consistent withreserve()on line 114)- Adding contextual information (e.g., queue size before/after)
- Removing it if not needed for diagnostics
Suggested change
- spdlog::info("thread_safe_queue_t<TItem>::pop_all()"); + spdlog::debug("thread_safe_queue_t<TItem>::pop_all() - size: {}", m_queue.size());src/include/db/manifest/manifest.h (1)
160-160: Clarify the rationale for shared WAL ownership in the constructor.The constructor change to accept
wal::shared_ptr_t<manifest_t::record_t>indicates a shift to shared ownership semantics. Consider documenting:
- Why shared ownership is necessary (e.g., multiple manifest instances reference the same WAL, WAL lifetime managed externally)
- Whether this represents a change in ownership model from previous implementations, and any migration concerns
No obvious thread-safety issues were identified: the WAL implementation does not show intrinsic concurrency mechanisms, but the manifest class itself has no concurrent access patterns. If the manifest is intended to be accessed from multiple threads, ensure the WAL implementation or calling code provides appropriate synchronization.
src/config/config.cpp (4)
17-18: Consider consistent initialization syntax.Line 17 uses braces
{}while line 18 uses parentheses()for initialization. For consistency, consider using the same style throughout.🔎 Proposed fix for consistency
static constexpr const std::string_view SegmentsDirectoryName{"segments"}; -static constexpr const std::string_view ManifestCurrentFilename("current"); +static constexpr const std::string_view ManifestCurrentFilename{"current"};
176-176: Use consistent error formatting with fmt::format.Line 176 uses string concatenation while other error messages in this file use
fmt::format(e.g., lines 204, 219, 244). Consider usingfmt::formatfor consistency.🔎 Proposed fix
- throw std::runtime_error("unable to create \"current\" " + current_path.string()); + throw std::runtime_error(fmt::format("unable to create \"current\" {}", current_path.string()));
199-207: Add error handling for JSON parse failures.Line 206 calls
nlohmann::json::parse()without explicit error handling. While parse exceptions will propagate, they won't include context about which config file failed. Adding a try-catch block with contextual error information would improve debugging.🔎 Proposed enhancement
auto loadConfigJson(const std::string &configPath) -> nlohmann::json { std::fstream configStream(configPath, std::fstream::in); if (!configStream.is_open()) { throw std::runtime_error(fmt::format("Unable to open config file: {}", configPath)); } - return nlohmann::json::parse(configStream); + + try + { + return nlohmann::json::parse(configStream); + } + catch (const nlohmann::json::parse_error &e) + { + throw std::runtime_error(fmt::format("Failed to parse JSON from {}: {}", configPath, e.what())); + } }
179-182: Consider decoupling config from LSM tree structures.The
configmodule depends onstructures::lsmtree::segments::helpers::uuid(), creating coupling between configuration and LSM tree internals. Consider moving the UUID generation to a common utilities module to maintain better separation of concerns.The relevant code snippet shows a UUID function exists in
src/common/helpers.cpp. Consider using that instead:- const auto new_filename{ - DatabaseConfig.DatabasePath / - fmt::format("manifest_{}", structures::lsmtree::segments::helpers::uuid()) - }; + const auto new_filename{ + DatabaseConfig.DatabasePath / + fmt::format("manifest_{}", common::helpers::uuid()) + };And update the include accordingly.
src/structures/memtable/memtable.cpp (1)
4-4: Unused include detected.The
spdlogheader is included but there are no visible logging calls in this file.🔎 Verify spdlog usage
#!/bin/bash # Check if spdlog is actually used in this file rg -n 'spdlog::|SPDLOG_' src/structures/memtable/memtable.cppsrc/include/serialization/buffer_reader.h (3)
6-6: Remove unused spdlog include.The spdlog header is included but never used in this file. Removing it will reduce compilation dependencies and improve build times.
🔎 Proposed fix
#include <span> #include <cstdint> -#include <spdlog/spdlog.h> #include <string>
26-26: Consider standardizing error checking pattern.The code mixes two patterns for checking error state:
- Direct check:
if (m_error)(lines 26, 43, 78)- Method call:
if (has_error())(lines 55, 91, 103)Both are functionally equivalent, but using a consistent pattern improves readability. Consider standardizing on
has_error()throughout, as it's more self-documenting.🔎 Proposed fix
template <EndianInteger T> [[nodiscard]] auto read_endian_integer(T &out) noexcept -> buffer_reader_t & { - if (m_error) + if (has_error()) { return *this; }Apply similar changes to lines 43 and 78.
Also applies to: 43-43, 55-55, 78-78, 91-91, 103-103
72-72: Consider renaming enum value for clarity.The enum value
invalid_variant_kis correctly named per the definition, but it's confusingly termed for this context. The method isread_varint(variable-length integer), so the enum should useinvalid_varint_kto align terminology. "Variant" typically refers to type variants/discriminated unions, not variable-length integers. Rename the enum insrc/include/serialization/common.hfrominvalid_variant_ktoinvalid_varint_kand update usages insrc/include/serialization/buffer_reader.handsrc/serialization/varint_test.cppaccordingly.src/structures/lsmtree/levels/level.cpp (4)
118-122: Prefer integer arithmetic overstd::powfor integer exponentiation.Using
std::powfor computing powers of 10 introduces floating-point arithmetic and potential precision issues. Sinceindex()is an integer, consider using integer-based exponentiation.🔎 Proposed refactor using integer arithmetic
- const std::uint64_t threshold{ - index() == 0 ? m_pConfig->LSMTreeConfig.LevelZeroCompactionThreshold - : m_pConfig->LSMTreeConfig.LevelNonZeroCompactionThreshold * - static_cast<std::uint64_t>(std::pow(10, index())) - }; + auto compute_power_of_10 = [](std::uint64_t exp) -> std::uint64_t { + std::uint64_t result = 1; + for (std::uint64_t i = 0; i < exp; ++i) { + result *= 10; + } + return result; + }; + const std::uint64_t threshold{ + index() == 0 ? m_pConfig->LSMTreeConfig.LevelZeroCompactionThreshold + : m_pConfig->LSMTreeConfig.LevelNonZeroCompactionThreshold * + compute_power_of_10(index()) + };Alternatively, you could use a precomputed lookup table or the standard library's
std::powwith explicit rounding, but integer arithmetic is clearest for this use case.
125-127: Remove or uncomment the dead code.The commented-out log statement should either be removed entirely or uncommented if the information is valuable for debugging. Leaving commented code in the codebase reduces readability.
194-200: Optional: Remove redundant return type annotation.The
-> autoreturn type annotation on the lambda is redundant since lambdas automatically deduce their return type. You can simplify to:[pSegment](const auto ¤tSegment)
241-243: Acknowledge the TODO for configurability.The hard-coded segment size of 1024 is noted in the TODO comment. Ensure this technical debt is tracked so the segment size becomes configurable as intended.
Do you want me to open a new issue to track making the segment size configurable?
src/structures/lsmtree/levels/levels.cpp (2)
80-101: LGTM with optional refactoring suggestion.The scoped pattern for manifest updates improves debuggability by capturing the result before asserting. The consistent application across multiple call sites is good.
For future consideration, you could reduce boilerplate with a helper function:
template<typename Record> void assert_manifest_add(db::manifest::shared_ptr_t& manifest, Record&& record) { auto ok = manifest->add(std::forward<Record>(record)); ASSERT(ok); }Then usage becomes:
assert_manifest_add(m_pManifest, db::manifest::manifest_t::level_record_t{ .op = level_operation_k::compact_level_k, .level = currentLevel->index() });Also applies to: 117-126, 190-195
185-185: Optional: Consider removing the redundant assertion.The assertion
ASSERT(m_levels[0])verifies that level 0 exists, but the constructor always creates 7 levels (lines 28-32). This assertion is unlikely to fail unless there's a severe logic error elsewhere. Consider whether it adds value or just adds noise.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
Dockerfileconan/profiles/debug-clangconan/profiles/debug-clang-tsanconan/profiles/debug-gccconan/profiles/release-clangconan/profiles/release-gccconanfile.txtdocs/sstable_binary_format_spec_v1.mdmise.tomlproto/CMakeLists.txtproto/raft/v1/raft_service.protorequirements.txtsrc/common/helpers.cppsrc/config/config.cppsrc/db/db.cppsrc/fs/append_only_file.cppsrc/fs/mmap_random_access_file.cppsrc/fs/random_access_file.cppsrc/include/common/helpers.hsrc/include/concurrency/thread_safe_queue.hsrc/include/config/config.hsrc/include/db/manifest/manifest.hsrc/include/fs/append_only_file.hsrc/include/fs/mmap_random_access_file.hsrc/include/serialization/buffer_reader.hsrc/include/serialization/endian_integer.hsrc/include/server/server_kind.hsrc/include/structures/lsmtree/segments/lsmtree_segment_factory.hsrc/include/structures/memtable/memtable.hsrc/include/structures/skiplist/skiplist.hsrc/raft/raft.cppsrc/serialization/CMakeLists.txtsrc/serialization/buffer_reader_writer_test.cppsrc/serialization/endian_integer_test.cppsrc/server/grpc_server.cppsrc/structures/lsmtree/levels/level.cppsrc/structures/lsmtree/levels/levels.cppsrc/structures/lsmtree/lsmtree.cppsrc/structures/lsmtree/lsmtree_test_basic.cppsrc/structures/lsmtree/segments/lsmtree_regular_segment.cppsrc/structures/lsmtree/segments/lsmtree_segment_factory.cppsrc/structures/memtable/memtable.cppsrc/structures/skiplist/CMakeLists.txtsrc/structures/skiplist/skiplist_test.cpptests/go/kvtest/cmd/kvtest/main.gotests/go/kvtest/pkg/core/context.gotests/go/kvtest/pkg/scenarios/readyourwrites.go
💤 Files with no reviewable changes (1)
- requirements.txt
✅ Files skipped from review due to trivial changes (1)
- proto/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (12)
- mise.toml
- tests/go/kvtest/cmd/kvtest/main.go
- src/serialization/buffer_reader_writer_test.cpp
- src/include/fs/append_only_file.h
- src/common/helpers.cpp
- src/structures/skiplist/skiplist_test.cpp
- tests/go/kvtest/pkg/core/context.go
- src/include/config/config.h
- src/serialization/endian_integer_test.cpp
- src/include/server/server_kind.h
- src/serialization/CMakeLists.txt
- conanfile.txt
🧰 Additional context used
🧬 Code graph analysis (14)
tests/go/kvtest/pkg/scenarios/readyourwrites.go (1)
tests/go/kvtest/pkg/core/context.go (1)
TestContext(10-22)
src/include/serialization/buffer_reader.h (4)
src/include/serialization/endian_integer.h (3)
serialization(13-73)from_bytes(51-62)result(55-58)src/include/serialization/concepts.h (1)
serialization(6-25)src/include/serialization/buffer_writer.h (4)
nodiscard(27-37)nodiscard(62-72)nodiscard(74-90)nodiscard(107-113)src/include/serialization/common.h (1)
MAX_VARINT_BYTES(34-48)
src/include/structures/lsmtree/segments/lsmtree_segment_factory.h (2)
src/structures/lsmtree/segments/lsmtree_segment_factory.cpp (2)
lsmtree_segment_factory(6-10)lsmtree_segment_factory(6-7)src/structures/lsmtree/segments/lsmtree_regular_segment.cpp (2)
memtable(479-482)memtable(479-479)
src/structures/lsmtree/segments/lsmtree_regular_segment.cpp (7)
src/include/serialization/buffer_reader.h (3)
nodiscard(41-74)nodiscard(76-90)nodiscard(100-111)src/include/serialization/buffer_writer.h (4)
nodiscard(27-37)nodiscard(62-72)nodiscard(74-90)nodiscard(107-113)src/serialization/crc32.cpp (2)
nodiscard(10-19)nodiscard(22-25)src/serialization/common.cpp (4)
nodiscard(6-9)nodiscard(11-19)nodiscard(21-30)varint_size(21-21)src/include/structures/lsmtree/segments/lsmtree_regular_segment.h (1)
regular_segment_t(13-71)src/structures/hashindex/hashindex.cpp (4)
offset(27-35)offset(27-27)num_of_bytes_used(37-40)num_of_bytes_used(37-37)src/fs/random_access_file.cpp (4)
fd(140-153)fd(140-140)flush(155-169)flush(155-155)
src/include/db/manifest/manifest.h (1)
src/include/wal/wal.h (1)
wal(15-74)
src/db/db.cpp (1)
src/structures/memtable/memtable.cpp (6)
record(84-91)record(84-84)record(93-96)record(93-93)record(98-101)record(98-98)
src/raft/raft.cpp (1)
src/raft/raft_test.cpp (1)
nodeConfig(44-67)
src/include/fs/mmap_random_access_file.h (2)
src/structures/lsmtree/lsmtree.cpp (2)
build(188-212)build(188-190)src/include/posix_wrapper/open_flag.h (2)
posix_wrapper(13-40)open_flag_k(16-34)
src/structures/lsmtree/levels/level.cpp (4)
src/common/helpers.cpp (4)
segment_name(26-29)segment_name(26-26)segment_path(31-35)segment_path(31-32)src/structures/lsmtree/segments/helpers.cpp (4)
segment_name(27-30)segment_name(27-27)segment_path(32-36)segment_path(32-33)src/structures/lsmtree/segments/lsmtree_segment_factory.cpp (2)
lsmtree_segment_factory(6-10)lsmtree_segment_factory(6-7)src/include/structures/lsmtree/levels/level.h (1)
purge(53-79)
src/structures/lsmtree/levels/levels.cpp (3)
src/common/helpers.cpp (2)
segment_name(26-29)segment_name(26-26)src/structures/lsmtree/segments/helpers.cpp (2)
segment_name(27-30)segment_name(27-27)src/structures/lsmtree/levels/level.cpp (2)
compact(112-181)compact(112-112)
src/structures/lsmtree/segments/lsmtree_segment_factory.cpp (1)
src/structures/lsmtree/segments/lsmtree_regular_segment.cpp (2)
memtable(479-482)memtable(479-479)
src/include/common/helpers.h (1)
src/common/helpers.cpp (10)
uuid(13-24)uuid(13-13)segment_name(26-29)segment_name(26-26)segment_path(31-35)segment_path(31-32)generateRandomString(37-59)generateRandomString(37-37)generateRandomStringPairVector(61-74)generateRandomStringPairVector(61-62)
src/config/config.cpp (1)
src/common/helpers.cpp (2)
uuid(13-24)uuid(13-13)
src/structures/lsmtree/lsmtree_test_basic.cpp (3)
src/structures/lsmtree/levels/level.cpp (2)
record(97-110)record(97-98)src/structures/lsmtree/levels/levels.cpp (2)
record(155-171)record(155-155)src/structures/memtable/memtable.cpp (6)
record(84-91)record(84-84)record(93-96)record(93-93)record(98-101)record(98-98)
🪛 markdownlint-cli2 (0.18.1)
docs/sstable_binary_format_spec_v1.md
8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| ``` | ||
| HEADER (24 bytes): | ||
| +--------+--------+--------+--------+--------+--------+ | ||
| | magic | ver | body | index | size | crc32 | | ||
| | (u32) | (u32) | offset | offset | (u32) | (u32) | | ||
| | bytes | bytes | (u32) | (u32) | bytes | bytes | | ||
| | 0-3 | 4-7 | 8-11 | 12-15 | 16-19 | 20-23 | | ||
| +--------+--------+--------+--------+--------+--------+ | ||
| BODY (variable): | ||
| +----------+-----+----------+----------+-----+-----------+ | ||
| | key_len | key | seq_num | val_len | val | timestamp | | ||
| | (varint) | var | (u64) | (varint) | var | (u32) | | ||
| +----------+-----+----------+----------+-----+-----------+ | ||
| Repeated for each entry | ||
| INDEX (variable): | ||
| +----------+-----+----------+ | ||
| | key_len | key | body_off | | ||
| | (varint) | var | (u32) | | ||
| +----------+-----+----------+ | ||
| Every entry (dense) or every k-th entry (sparse) | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add language specifications to fenced code blocks per markdown style guidelines.
All four code blocks lack explicit language identifiers, triggering linting warnings. While the content is clear, adding language hints improves rendering and follows markdown best practices.
🔎 Proposed fix: Add language identifiers
-```
+```text
HEADER (24 bytes):-```
+```c
Header {
u32 magic = 0xDEADBEEF;-```
+```pseudocode
Body {
for each entry in table:-```
+```pseudocode
Index {
for each entry in table: // dense initiallyAlso applies to: 36-45, 49-59, 63-70
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/sstable_binary_format_spec_v1.md around lines 8-30 (also apply same
change to blocks at 36-45, 49-59, and 63-70): the fenced code blocks lack
language identifiers; update each opening fence to include an appropriate
language hint (e.g., use ```text for the HEADER diagram, ```c for the Header
struct example, and ```pseudocode for the BODY and INDEX examples) so markdown
linters/renderers recognize the block types and the file conforms to style
guidelines.
| #include <filesystem> | ||
| #include <random> | ||
| #include <vector> | ||
|
|
||
| #include <fmt/core.h> |
There was a problem hiding this comment.
Missing required standard library includes.
The header declares functions returning std::string and using std::numeric_limits and std::pair, but the necessary headers are missing:
<string>forstd::string(used in lines 37, 39, 44, 46-47)<limits>forstd::numeric_limits(used in line 14-15)<utility>forstd::pair(used in line 47)
🔎 Proposed fix
#include <filesystem>
+#include <limits>
#include <random>
+#include <string>
+#include <utility>
#include <vector>
#include <fmt/core.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <filesystem> | |
| #include <random> | |
| #include <vector> | |
| #include <fmt/core.h> | |
| #include <filesystem> | |
| #include <limits> | |
| #include <random> | |
| #include <string> | |
| #include <utility> | |
| #include <vector> | |
| #include <fmt/core.h> |
🤖 Prompt for AI Agents
In src/include/common/helpers.h around lines 3 to 7, the header is missing
required standard includes for types and utilities used later; add #include
<string> for std::string, #include <limits> for std::numeric_limits, and
#include <utility> for std::pair (placed with the other standard includes near
the top of the file) so declarations using those types compile correctly.
| template <typename TNumber> | ||
| auto generateRandomNumber( | ||
| const TNumber min = std::numeric_limits<TNumber>::min(), | ||
| const TNumber max = std::numeric_limits<TNumber>::max() | ||
| ) noexcept -> TNumber | ||
| { | ||
| static thread_local std::mt19937 rng{std::random_device{}()}; | ||
|
|
||
| if constexpr (std::is_integral_v<TNumber>) | ||
| { | ||
| return std::uniform_int_distribution<TNumber>(min, max)(rng); | ||
| } | ||
| else if constexpr (std::is_floating_point_v<TNumber>) | ||
| { | ||
| return std::uniform_real_distribution<TNumber>(min, max)(rng); | ||
| } | ||
| else | ||
| { | ||
| static_assert( | ||
| std::is_integral_v<TNumber> || std::is_floating_point_v<TNumber>, | ||
| "TNumber must be an integral or floating-point type" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Weak RNG seeding reduces randomness quality.
Line 18 seeds the mt19937 with a single 32-bit value from std::random_device{}(), providing only 32 bits of entropy for the generator's 19937-bit internal state. This is a well-known pitfall that significantly reduces randomness quality.
By contrast, the uuid() implementation in src/common/helpers.cpp (lines 12-23) properly seeds the generator by filling an array of size mt19937::state_size with random values and using std::seed_seq. Consider adopting the same seeding strategy here for consistency and quality.
🔎 Proposed fix for proper seeding
template <typename TNumber>
auto generateRandomNumber(
const TNumber min = std::numeric_limits<TNumber>::min(),
const TNumber max = std::numeric_limits<TNumber>::max()
) noexcept -> TNumber
{
- static thread_local std::mt19937 rng{std::random_device{}()};
+ static thread_local auto rng = []() {
+ std::random_device rd;
+ std::array<int, std::mt19937::state_size> seed_data;
+ std::generate(seed_data.begin(), seed_data.end(), std::ref(rd));
+ std::seed_seq seq(seed_data.begin(), seed_data.end());
+ return std::mt19937{seq};
+ }();
if constexpr (std::is_integral_v<TNumber>)
{Note: This requires #include <array> and #include <algorithm> (for std::generate).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <typename TNumber> | |
| auto generateRandomNumber( | |
| const TNumber min = std::numeric_limits<TNumber>::min(), | |
| const TNumber max = std::numeric_limits<TNumber>::max() | |
| ) noexcept -> TNumber | |
| { | |
| static thread_local std::mt19937 rng{std::random_device{}()}; | |
| if constexpr (std::is_integral_v<TNumber>) | |
| { | |
| return std::uniform_int_distribution<TNumber>(min, max)(rng); | |
| } | |
| else if constexpr (std::is_floating_point_v<TNumber>) | |
| { | |
| return std::uniform_real_distribution<TNumber>(min, max)(rng); | |
| } | |
| else | |
| { | |
| static_assert( | |
| std::is_integral_v<TNumber> || std::is_floating_point_v<TNumber>, | |
| "TNumber must be an integral or floating-point type" | |
| ); | |
| } | |
| } | |
| template <typename TNumber> | |
| auto generateRandomNumber( | |
| const TNumber min = std::numeric_limits<TNumber>::min(), | |
| const TNumber max = std::numeric_limits<TNumber>::max() | |
| ) noexcept -> TNumber | |
| { | |
| static thread_local auto rng = []() { | |
| std::random_device rd; | |
| std::array<int, std::mt19937::state_size> seed_data; | |
| std::generate(seed_data.begin(), seed_data.end(), std::ref(rd)); | |
| std::seed_seq seq(seed_data.begin(), seed_data.end()); | |
| return std::mt19937{seq}; | |
| }(); | |
| if constexpr (std::is_integral_v<TNumber>) | |
| { | |
| return std::uniform_int_distribution<TNumber>(min, max)(rng); | |
| } | |
| else if constexpr (std::is_floating_point_v<TNumber>) | |
| { | |
| return std::uniform_real_distribution<TNumber>(min, max)(rng); | |
| } | |
| else | |
| { | |
| static_assert( | |
| std::is_integral_v<TNumber> || std::is_floating_point_v<TNumber>, | |
| "TNumber must be an integral or floating-point type" | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/include/common/helpers.h around lines 12 to 35, the thread_local mt19937
is seeded with a single 32-bit value from std::random_device which provides
insufficient entropy for mt19937; replace the simple seed with a std::seed_seq
initialized from an array of size std::mt19937::state_size filled by repeated
calls to std::random_device (use std::generate or a loop), then construct the
thread_local mt19937 with that seed_seq for proper seeding; also add the
required headers (#include <array> and #include <algorithm>) to the file.
| auto regular_segment_t::min() const noexcept -> std::optional<record_t::key_t> | ||
| { | ||
| return m_memtable->min(); | ||
| } | ||
|
|
||
| std::optional<record_t::key_t> regular_segment_t::max() const noexcept | ||
| { | ||
|
|
||
| return m_memtable->max(); | ||
| } |
There was a problem hiding this comment.
Potential null dereference in min()/max().
Both min() and max() dereference m_memtable using -> without first checking has_value(). If called when m_memtable is empty (e.g., after moved_memtable()), this will cause undefined behavior.
Proposed fix
auto regular_segment_t::min() const noexcept -> std::optional<record_t::key_t>
{
- return m_memtable->min();
+ return m_memtable.has_value() ? m_memtable->min() : std::nullopt;
}
std::optional<record_t::key_t> regular_segment_t::max() const noexcept
{
-
- return m_memtable->max();
+ return m_memtable.has_value() ? m_memtable->max() : std::nullopt;
}🤖 Prompt for AI Agents
In src/structures/lsmtree/segments/lsmtree_regular_segment.cpp around lines 452
to 461, min() and max() dereference m_memtable without checking for emptiness
which can null-dereference after moved_memtable(); fix by testing if
m_memtable.has_value() (or equivalent) first and return std::nullopt when it is
not set, otherwise call m_memtable->min() / m_memtable->max(); keep the
functions noexcept and preserve the existing return types.
| void regular_segment_t::restore() | ||
| { | ||
| // TODO(lnikon): Do we need this condition? | ||
| if (!m_memtable->empty()) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential null dereference in restore().
m_memtable->empty() is called without first checking has_value(). If m_memtable is std::nullopt, this causes undefined behavior.
Proposed fix
void regular_segment_t::restore()
{
- // TODO(lnikon): Do we need this condition?
- if (!m_memtable->empty())
+ if (m_memtable.has_value() && !m_memtable->empty())
{
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void regular_segment_t::restore() | |
| { | |
| // TODO(lnikon): Do we need this condition? | |
| if (!m_memtable->empty()) | |
| { | |
| return; | |
| } | |
| void regular_segment_t::restore() | |
| { | |
| if (m_memtable.has_value() && !m_memtable->empty()) | |
| { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/structures/lsmtree/segments/lsmtree_regular_segment.cpp around lines
489-495, restore() calls m_memtable->empty() without confirming m_memtable has a
value, risking null dereference; update the condition to first check
m_memtable.has_value() (or operator bool) and only call empty() when present —
for example, return early if m_memtable is not set or if the memtable exists and
is not empty so the empty() call is guarded by the presence check.
| if (m_hashIndex.empty()) | ||
| { | ||
| spdlog::warn("(restore_index): Hash index is empty for segment {}", m_path.c_str()); | ||
| ASSERT(!m_hashIndex.empty()); | ||
| } |
There was a problem hiding this comment.
ASSERT after warn will crash on empty index.
The logic warns that the hash index is empty, then immediately asserts it must not be empty. If restore_index() legitimately results in an empty index (e.g., empty segment file), this will crash. Consider returning early or handling the empty case gracefully.
Proposed fix
if (m_hashIndex.empty())
{
spdlog::warn("(restore_index): Hash index is empty for segment {}", m_path.c_str());
- ASSERT(!m_hashIndex.empty());
+ // Early return - nothing to restore from an empty index
+ return;
}
}Alternatively, if an empty index after restoration is truly a bug, change the warn to an error before the assert.
🤖 Prompt for AI Agents
In src/structures/lsmtree/segments/lsmtree_regular_segment.cpp around lines
680-684, the code logs a warning when m_hashIndex is empty and then immediately
ASSERTs, which will crash for legitimately empty indexes; change the control
flow to handle the empty case gracefully by replacing the ASSERT with an early
return (or return a failure status) after the warning and ensure the caller
handles an empty index, or if empty state is truly fatal, change the
spdlog::warn to spdlog::error before keeping the ASSERT so the log level matches
the fatal assertion.
| auto memtable_t::record_t::operator==(const record_t &record) const -> bool | ||
| { | ||
| return m_key == record.m_key; | ||
| return m_key == record.m_key && m_sequenceNumber == record.m_sequenceNumber; | ||
| } |
There was a problem hiding this comment.
Critical: operator== is inconsistent with operator<.
The operator== now requires both m_key and m_sequenceNumber to match, while operator< uses both for ordering. This violates the strict weak ordering requirement for sorted containers:
Problem: For two records with the same key but different sequence numbers:
!(a < b)and!(b < a)would both be false (they differ by sequenceNumber)- But
a == bwould also be false
This violates the requirement that !(a < b) && !(b < a) implies a == b, which is essential for correct behavior of std::set, std::map, and other associative containers.
Impact: Undefined behavior in sorted containers, potential crashes, incorrect lookups, and data corruption.
🔎 Suggested fix
Choose one approach:
Option 1: Make equality consistent with ordering (recommended if you want multiple versions per key):
auto memtable_t::record_t::operator==(const record_t &record) const -> bool
{
- return m_key == record.m_key && m_sequenceNumber == record.m_sequenceNumber;
+ return !((*this < record) || (record < *this));
}Option 2: Compare only keys in both operators (if sequence number is metadata):
auto memtable_t::record_t::operator<(const memtable_t::record_t &record) const -> bool
{
- if (m_key != record.m_key)
- {
- return m_key < record.m_key;
- }
- return m_sequenceNumber < record.m_sequenceNumber;
+ return m_key < record.m_key;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto memtable_t::record_t::operator==(const record_t &record) const -> bool | |
| { | |
| return m_key == record.m_key; | |
| return m_key == record.m_key && m_sequenceNumber == record.m_sequenceNumber; | |
| } | |
| auto memtable_t::record_t::operator==(const record_t &record) const -> bool | |
| { | |
| return !((*this < record) || (record < *this)); | |
| } |
🤖 Prompt for AI Agents
In src/structures/memtable/memtable.cpp around lines 98-101, operator== is
inconsistent with operator< which breaks strict weak ordering; change operator==
to be defined in terms of operator< (e.g. return !(*this < record) && !(record <
*this)) so equality matches the ordering used by associative containers,
ensuring !(a<b) && !(b<a) implies a==b.
| @@ -0,0 +1,22 @@ | |||
| cmake_minimum_required(VERSION 3.25) | |||
| project(frankie) | |||
There was a problem hiding this comment.
Remove the project() call from subdirectory CMakeLists.txt.
Redefining project() in a subdirectory CMakeLists.txt creates a separate project scope and can override parent project settings. Additionally, the project name "frankie" doesn't match the repository name "tinykvpp", suggesting a copy-paste error.
🔎 Proposed fix
-project(frankie)Remove this line entirely. The project should only be defined in the top-level CMakeLists.txt.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| project(frankie) |
🤖 Prompt for AI Agents
In src/structures/skiplist/CMakeLists.txt around line 2, remove the stray
project(frankie) call: this subdirectory CMakeLists should not redefine
project() (and the name "frankie" is incorrect for this repo); delete that line
so the project is only declared in the top-level CMakeLists.txt and commit the
change.
- Fixing issues found during code review
Summary by CodeRabbit
Release Notes
New Features
Infrastructure & Build
Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.