-
Notifications
You must be signed in to change notification settings - Fork 5
MOD-11322 Adjust to new ASM API #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/cluster.c
Outdated
|
|
||
| int MR_ClusterIsMySlot(size_t slot) { | ||
| if (RedisModule_ShardingGetSlotRange) { | ||
| if (RedisModule_ClusterCanAccessKeysInSlot != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I wasn't expecting this part in libmr. Thank you for catching it.
Even if we have RedisModule_ClusterCanAccessKeysInSlot(), we may need to use RedisModule_ShardingGetSlotRange() if this is non-asm RE build with Redis 8.4+ :(
Libmr needs to know what kind of clustering is this. I'm not familiar with libmr but I'll try to check it later.
In summary,
- For OSS, we should call
RedisModule_ClusterCanAccessKeysInSlot()if it exists. Otherwise, fallback toclusterCtx.minSlotandclusterCtx.maxSlot. (I'm not sure about this yet, perhaps you may have an idea) - For RE, if this is not OSS clustering, we should call
RedisModule_ShardingGetSlotRange(), otherwise, we'll callRedisModule_ClusterCanAccessKeysInSlot(). I'm not sure how libmr can detect this yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the ask is to adjust to oss cluster only. The adjustments needed for RE with oss-cluster bdbs will come next.
So I'll rely on clusterCtx.isOss for now and will add whatever code is needed for RE ASMs under another ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. Tomorrow, I'll consult others to confirm how we can handle this, maybe we can do something like:
if (RedisModule_ShardingGetSlotRange) {
int first_slot, last_slot;
RedisModule_ShardingGetSlotRange(&first_slot, &last_slot);
if (first_slot != -1 && last_slot != -1) {
/* RE resharding is enabled */
return first_slot <= slot && last_slot >= slot;
}
/* RE sharding is not enabled, try other methods.. */
}
if (RedisModule_ClusterCanAccessKeysInSlot != NULL)
return RedisModule_ClusterCanAccessKeysInSlot(slot);
// Fallback.
return clusterCtx.minSlot <= slot && clusterCtx.maxSlot >= slot;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
Note: For RE builds, we need adjustment in MR_ClusterIsMySlot() to detect if RE resharding or OSS clustering is used.
Indeed, but that would be under https://redislabs.atlassian.net/browse/MOD-11623 |
f308f66 to
0859b00
Compare
72d98ac to
93b1ace
Compare
Note
Update slot-ownership logic to use
RedisModule_ClusterCanAccessKeysInSloton OSS and extendredismodule.hwith new cluster migration events/structs, keyspace flags, config iterator/get/set, and related APIs wired into init.MR_ClusterIsMySlotto useRedisModule_ClusterCanAccessKeysInSloton OSS; enterprise usesRedisModule_ShardingGetSlotRange; fallback to cachedclusterCtxslot range.redismodule.h):REDISMODULE_NOTIFY_OVERWRITTEN,REDISMODULE_NOTIFY_TYPE_CHANGED,REDISMODULE_NOTIFY_KEY_TRIMMED; bump_REDISMODULE_NOTIFY_NEXT.REDISMODULE_EVENT_CLUSTER_SLOT_MIGRATIONand..._TRIMwith subevents; exposeRedisModuleEvent_ClusterSlotMigration*.RedisModuleSlotRange{,Array},RedisModuleClusterSlotMigrationInfo,RedisModuleClusterSlotMigrationTrimInfo.RedisModuleConfigType,RedisModuleConfigIteratorand config get/set helpers.RedisModule_ClusterCanAccessKeysInSlot,...ClusterPropagateForSlotMigration,...ClusterGetLocalSlotRanges,...ClusterFreeSlotRanges.RedisModule_UnsubscribeFromKeyspaceEvents.REDISMODULE_GET_API.Written by Cursor Bugbot for commit 93b1ace. This will update automatically on new commits. Configure here.