Add support for MustObeyClient Module API #1582
Conversation
Signed-off-by: KarthikSubbarao <[email protected]>
Signed-off-by: KarthikSubbarao <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1582 +/- ##
============================================
+ Coverage 70.94% 70.95% +0.01%
============================================
Files 121 121
Lines 65132 65136 +4
============================================
+ Hits 46207 46218 +11
+ Misses 18925 18918 -7
|
| * and should never be rejected. | ||
| * Returns 0 otherwise (and also if ctx or if the client is NULL). */ | ||
| int VM_MustObeyClient(ValkeyModuleCtx *ctx) { | ||
| if (!ctx || !ctx->client) return 0; |
There was a problem hiding this comment.
My only fear is that it will become confusing since many times the context holds a fake/tmp client which will not be obeyed right? I think this is why in most cases were module API was checking the mustObeyClient it was targeting the server.current_client. Not sure this is a big concern but maybe we should allow somehow to also check the current running client? like int VM_MustObeyCurrentRunningClient()?
Given said that. I do agree fake clients and tmp clients are usually used during background/blocking operations which might not be relevant for AOF/replication flows so maybe we can consider that in the future.
There was a problem hiding this comment.
Yeah, I looked through the code and didn't see any specific examples where we would need to find the real client. Replication from the primary should not block or rely on background behavior.
| /* Returns 1 if commands are arriving from the primary client or AOF client | ||
| * and should never be rejected. | ||
| * Returns 0 otherwise (and also if ctx or if the client is NULL). */ |
There was a problem hiding this comment.
Since this documentation is used to generate the public facing docs, I think it would also be useful to document why one would use this. Specifically it should be used for validation that might cause divergence on primaries and replicas (or AOF files).
This PR adds support for MustObeyClient Module API.
The purpose of this API is for Modules to handle commands / callbacks knowing whether commands are arriving from the primary client or AOF client and should never be rejected.
A use case is that Modules can have validation logic in command handlers which only should be executed on primary nodes. Replica nodes must obey commands replicated.
Will add tests once/if we agree on the use case and syntax of the Module API