Core/FFI: MonitorClient for MONITOR command#5977
Open
affonsov wants to merge 2 commits into
Open
Conversation
5 tasks
- glide-core/src/client/monitor_client.rs: dedicated non-pooled MonitorClient using redis::aio::Monitor, streams lines via mpsc channel, stop() via oneshot - glide-core/src/client/mod.rs: pub mod monitor_client + re-export MonitorClient - ffi/src/lib.rs: create_monitor_client() and close_monitor_client() C exports - glide-core/tests/test_monitor.rs: integration tests for start, receive, stop Signed-off-by: affonsov <67347924+affonsov@users.noreply.github.com>
affonsov
force-pushed
the
affonsov/monitor-core
branch
from
e165aff to
50f6844
Compare
affonsov
changed the title
- create_monitor_client now reads address/tls/auth directly from the protobuf ConnectionRequest before .into() conversion, avoiding access to fields absent in the mock-glide-core stub - adds MonitorClient, MonitorLine, MonitorLineCallback stubs to mock-glide-core/src/client.rs for miri-test compilation - adds NodeAddress/TlsMode internal-type stubs to mock client module - adds ProtocolVersion and RedisConnectionInfo stubs to mock-redis Signed-off-by: affonsov <67347924+affonsov@users.noreply.github.com>
yipin-chen
reviewed
May 25, 2026
| .expect("MonitorClient::new failed"); | ||
|
|
||
| monitor.stop(); | ||
| monitor.stop(); // must not panic |
jduo
reviewed
May 26, 2026
| assert!(!set_line.client_addr.is_empty()); | ||
| assert!(set_line.timestamp > 0.0); | ||
|
|
||
| monitor.stop(); |
Collaborator
There was a problem hiding this comment.
Should this be stop_async()?
| // Convert to internal ConnectionRequest (needed for the runtime type alias only). | ||
| let _connection_request: ConnectionRequest = connection_request.into(); | ||
|
|
||
| let runtime = match Builder::new_multi_thread().enable_all().build() { |
Collaborator
There was a problem hiding this comment.
Do we truly need a new multi-thread runtime for each monitor? I only see it used with block_on. Could we get away with using new_current_thread()?
Should this depend on if the client is sync or async (or does that even have meaning for a monitor?)
Collaborator
There was a problem hiding this comment.
I see, it's callback based so new_current_thread() wouldn't work here. Is there any relationship between the MonitorClient and GlideClient or GlideClusterClient? The Glide clients already have a background runtime that this work can be done on, but it doesn't seem like you have to create one to use a monitor.
In practice, is someone using a monitor going to need a Glide client too though?
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
3 participants
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
Adds a dedicated
MonitorClientto glide-core and exposes it via FFI for language client bindings.Issue link
This Pull Request is linked to issue: Implement MONITOR command: MonitorClient, FFI bindings, and language client wrappers
Closes #5978
Features / Behaviour Changes
MonitorClientinglide-core/src/client/monitor_client.rs: dedicated non-pooled connection usingredis::aio::Monitor, forces RESP2, streams parsedMonitorLinestructs via callbackMonitorLine { timestamp, db, client_addr, command, args }withparse()from raw monitor line stringsMonitorLineCallback = Arc<dyn Fn(MonitorLine) + Send + Sync>for Rust consumerscreate_monitor_client()andclose_monitor_client()inffi/src/lib.rsMonitorCallbackpasses individual parsed fields +args_json(JSON array) to C callersstop_async(self)awaits background task exit;ManuallyDrop+Runtime::block_onin FFI Drop ensures no use-after-freeImplementation
glide-core/src/client/monitor_client.rs(new): MonitorClient, MonitorLine, MonitorLineCallbackglide-core/src/client/mod.rs:pub mod monitor_client; pub use monitor_client::MonitorClient;ffi/src/lib.rs:create_monitor_client,close_monitor_client, MonitorAdapterglide-core/tests/test_monitor.rs(new): 3 integration testsLimitations
get_async_monitor()— only available path for a dedicated non-pooled connectionTesting
test_monitor_start_and_receive_line: connects monitor, issues SET, asserts all 5 parsed fieldstest_monitor_stop_no_lines_after_stop: callsstop_async().await, issues post-stop command, asserts no new linestest_monitor_stop_idempotent: verifies double-stop doesn't panicChecklist