ChronoKVS: Support configuration files#512
Draft
EnekoGonzalez3 wants to merge 22 commits intodevelopfrom
Draft
Conversation
…id time ranges. Update integration tests to verify event count and range correctness.
… event by key. Enhance integration tests to validate functionality and ensure correct event retrieval.
…st event by key. Update integration tests to validate functionality and ensure correct event retrieval.
Resolves merge conflicts and addresses Copilot review comments: - Extract MIN_TIMESTAMP/MAX_TIMESTAMP to file-level constants - Add documentation comments for get_range(), get_earliest(), and get_latest() APIs - Move <algorithm> include from header to cpp file - Fix misleading "All range retrievals successful" message - Update history_events comment to mention subsequent tests
The files were accidentally deleted during the merge from develop. These files are required by ChronoStore/test/CMakeLists.txt.
- Add spdlog-based logging throughout ChronoKVS plugin with [ChronoKVS] component tags - Add input validation for empty keys, empty values, and invalid timestamp ranges - Add error code propagation using chronolog::to_string() for meaningful error context - Add connection/disconnection logging at info level - Add operation logging at debug level for put, get, get_history, get_range, get_earliest, get_latest - Add error logging at error level for all failure conditions - Replace generic exceptions with error code context for debugging - Improve error messages with chronolog error code information
- Remove chrono_monitor.h dependency, use std::cerr for logging - Remove client_errcode.h dependency, use error codes directly - Remove chronolog_client link from integration test - Remove logger initialization from integration test ChronoKVS should only use the client API, not internal headers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements story handle caching in ChronoKVSClientAdapter to reduce network overhead and improve performance for repeated operations on the same keys. Changes: - Add handleCache (unordered_map) to store active handles by key - Add cacheMutex for thread-safe cache access - Add getOrAcquireHandle() helper for lazy handle acquisition - Modify storeEvent() and retrieveEvents() to use cached handles - Update destructor to release all cached handles before disconnect This addresses the performance issue where each put/get operation was acquiring and releasing a story handle, causing unnecessary RPC calls. With caching, 100 puts to the same key now require 1 acquire + 100 logs + 1 release instead of 100 acquire/release cycles. Closes #431
- Add constructor overloads accepting config_path parameter to ChronoKVS, ChronoKVSMapper, and ChronoKVSClientAdapter - Load configuration from JSON file using ClientConfiguration::load_from_file() when path is provided - Default constructor maintains backwards compatibility (uses localhost:5555) - Update writer example to show both local and distributed usage patterns
This comment was marked as resolved.
This comment was marked as resolved.
6 tasks
6 tasks
- Add command-line argument parsing for -c config_file in all ChronoKVS programs - Update chronokvs_integration_test to accept optional -c parameter - Update chronokvs_reader_example to accept optional -c parameter - Update chronokvs_writer_example to accept optional -c parameter - Programs work with and without -c flag (default uses localhost:5555)
This comment was marked as resolved.
This comment was marked as resolved.
Resolve conflicts by keeping develop's changes (get_earliest API, UINT64_MAX for MAX_TIMESTAMP, formatting) and adding get_latest API on top. Key changes: - Use UINT64_MAX for MAX_TIMESTAMP (from develop) - Keep get_earliest() implementation (from develop) - Add get_latest() API for retrieving most recent events - Add Test 6 for get_latest operations in integration test - Config file support works for both local and distributed deployments Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When CreateChronicle fails with an unexpected error code, properly disconnect and reset the client before throwing an exception. This ensures resources are properly cleaned up in error paths. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When Test 2 (Get History) throws an exception, random_indices remains empty. Test 3 was running anyway because it only checked timestamps.size(), causing a segfault when accessing the empty random_indices vector. Added check for random_indices.size() > 0 before running Test 3. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The UINT64_MAX value (from develop merge) causes ChronoLog query service to timeout. Reverted to the original value 2000000000000000000 which works correctly with ChronoLog's query implementation. This fixes the "Failed to replay events with error code: -12" (CL_ERR_QUERY_TIMED_OUT) issue in get_history and other read operations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Story handle caching (PR #431) broke read operations. The ChronoLog replay mechanism requires story handles to be released after read operations, but handle caching kept them open indefinitely. Changed retrieveEvents() to: 1. Temporarily acquire story handle (not cached) 2. Perform ReplayStory operation 3. Immediately release handle using RAII guard This matches the original working behavior before handle caching. Write operations (storeEvent) still use cached handles for performance. Also removed unnecessary skip logic from Test 3 - tests should run. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enable ChronoKVS to load client configuration from JSON files via
-cparameter, allowing connection to distributed ChronoLog deployments. All ChronoKVS programs now support both local (default) and distributed configurations.Changes (relative to #431)
Add optional
config_pathparameter throughout the ChronoKVS stack with-ccommand-line support:API Layer
ChronoKVS(const std::string& config_path = "")ChronoKVSMapper(const std::string& config_path = "")ChronoKVSClientAdapter(const std::string& config_path = "")Command-Line Support
All ChronoKVS programs now accept
-c config_fileparameter:chronokvs_integration_test [-c config_file]chronokvs_writer_example [-c config_file]chronokvs_reader_example [-c config_file]When
config_pathis provided, configuration is loaded usingClientConfiguration::load_from_file(). Without-c, programs use default localhost:5555 configuration.Usage
Default (localhost)
Distributed (with config file)
./chronokvs_integration_test -c ~/chronolog-install/chronolog/conf/default_client_conf.jsonProgrammatic API
Testing
Files Modified
chronokvs.h/chronokvs.cpp- Public API constructorchronokvs_mapper.h/chronokvs_mapper.cpp- Mapper layer constructorchronokvs_client_adapter.h/chronokvs_client_adapter.cpp- Adapter with config loadingchronokvs_writer_example.cpp- Command-line parsingchronokvs_reader_example.cpp- Command-line parsingchronokvs_integration_test.cpp- Command-line parsingDependencies
Closes #470