feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL#486
feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL#486
Conversation
…to Redis data Redis string keys were stored as bare keys (e.g., "foo"), making them indistinguishable from DynamoDB, S3, and other namespaces. This caused FLUSHALL to delete all data across all services. - Add !redis|str| prefix to all Redis string key read/write/delete paths - Update FLUSHALL to only delete !redis|* and !lst|* keys - Add FLUSHLEGACY command to scan and delete old unprefixed string keys
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates Redis string keys to use a "!redis|str|" prefix and introduces a FLUSHLEGACY command to remove old, unprefixed keys. Feedback identifies critical logic errors in the transaction load and buildKeyElems functions where unconditional prefixing causes double-prefixing for internal keys. Furthermore, the FLUSHLEGACY command requires batching to prevent potential OOM crashes and Raft message size violations caused by unbounded key scanning.
- Fix txnContext.load() to avoid double-prefixing internal keys (e.g., !redis|hash|) — only apply !redis|str| prefix to bare user keys - Fix buildKeyElems() to use storage keys from working map as-is - Fix stagedStringType() to look up by storage key - Batch FLUSHLEGACY scan+delete in groups of 1000 to avoid OOM and oversized Raft proposals
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a prefixing scheme for Redis string keys using the !redis|str| prefix and adds a FLUSHLEGACY command to remove old, unprefixed keys. Various Redis operations, internal state loading, and tests have been updated to use the new prefixed key format. A review comment suggests increasing the redisFlushLegacyTimeout from 60 seconds to 10 minutes to ensure sufficient time for cleaning up large legacy datasets.
adapter/redis.go
Outdated
| const ( | ||
| redisLatestCommitTimeout = 5 * time.Second | ||
| redisDispatchTimeout = 10 * time.Second | ||
| redisFlushLegacyTimeout = 60 * time.Second |
There was a problem hiding this comment.
The redisFlushLegacyTimeout of 60 seconds might be insufficient for large datasets. While increasing this value to 10 minutes provides a buffer, ensure that the underlying operation uses context.Context for deadline management as per repository standards. This avoids ambiguity and allows for better cancellation control, rather than relying solely on a hardcoded time.Duration constant or parameter.
| redisFlushLegacyTimeout = 60 * time.Second | |
| redisFlushLegacyTimeout = 10 * time.Minute |
References
- When designing interfaces, use context.Context for managing deadlines and cancellation. Avoid providing a separate time.Duration timeout parameter in method signatures to prevent ambiguity regarding which timeout takes precedence.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a migration to prefix Redis string keys with "!redis|str|" to separate them from other internal metadata. It introduces a FLUSHLEGACY command for one-time cleanup of unprefixed keys and updates core operations like GET, SET, and INCR to use the new prefixed keys. Feedback highlights a logic error in the FLUSHLEGACY scan range which would skip most legacy keys, and suggests a more robust check for internal keys in the load method to avoid incorrectly prefixing other internal namespaces.
adapter/redis_compat_commands.go
Outdated
| var cursor []byte | ||
| for { | ||
| kvs, err := r.store.ScanAt(ctx, cursor, []byte("!"), batchSize, readTS) | ||
| if err != nil { | ||
| conn.WriteError(fmt.Sprintf("ERR scan: %s", err)) | ||
| return | ||
| } | ||
| if len(kvs) == 0 { | ||
| break | ||
| } | ||
|
|
||
| elems := make([]*kv.Elem[kv.OP], 0, len(kvs)) | ||
| for _, pair := range kvs { | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key}) | ||
| } | ||
|
|
||
| if err := r.dispatchElems(ctx, false, readTS, elems); err != nil { | ||
| conn.WriteError(err.Error()) | ||
| return | ||
| } | ||
| totalDeleted += len(elems) | ||
|
|
||
| // Advance cursor past the last key in this batch by appending a | ||
| // zero byte to create a key strictly greater than the last one. | ||
| lastKey := kvs[len(kvs)-1].Key | ||
| cursor = make([]byte, len(lastKey)+1) | ||
| copy(cursor, lastKey) | ||
| } |
There was a problem hiding this comment.
The flushlegacy command as implemented will fail to delete most legacy keys. The scan range [nil, "!") only covers keys that are lexicographically smaller than !. Since most user keys (e.g., starting with alphanumeric characters) are greater than !, they will be skipped.
To correctly identify all legacy unprefixed keys, the scan should cover the entire keyspace and filter out keys starting with the internal prefix !. For better performance in large stores, you could perform two separate scans: [nil, "!") and ["\"", nil], skipping the internal ! block entirely.
var cursor []byte
for {
kvs, err := r.store.ScanAt(ctx, cursor, nil, batchSize, readTS)
if err != nil {
conn.WriteError(fmt.Sprintf("ERR scan: %s", err))
return
}
if len(kvs) == 0 {
break
}
elems := make([]*kv.Elem[kv.OP], 0, len(kvs))
for _, pair := range kvs {
if len(pair.Key) == 0 || pair.Key[0] != '!' {
elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key})
}
}
if len(elems) > 0 {
if err := r.dispatchElems(ctx, false, readTS, elems); err != nil {
conn.WriteError(err.Error())
return
}
totalDeleted += len(elems)
}
lastKey := kvs[len(kvs)-1].Key
cursor = make([]byte, len(lastKey)+1)
copy(cursor, lastKey)
}| storageKey := key | ||
| userKey := extractRedisInternalUserKey(key) | ||
| if userKey == nil && !isRedisTTLKey(key) { | ||
| storageKey = redisStrKey(key) | ||
| userKey = key | ||
| } |
There was a problem hiding this comment.
The logic for determining if a key is an internal Redis key is incomplete and fragile. It currently relies on extractRedisInternalUserKey, which only checks for !redis| prefixes, and an explicit check for TTL keys. This will incorrectly prefix other internal namespaces like !lst| (lists) or !txn| (transaction metadata) with !redis|str|.
A more robust approach is to check if the key starts with the internal prefix character ! directly.
| storageKey := key | |
| userKey := extractRedisInternalUserKey(key) | |
| if userKey == nil && !isRedisTTLKey(key) { | |
| storageKey = redisStrKey(key) | |
| userKey = key | |
| } | |
| storageKey := key | |
| userKey := extractRedisInternalUserKey(key) | |
| if !bytes.HasPrefix(key, []byte("!")) { | |
| storageKey = redisStrKey(key) | |
| userKey = key | |
| } |
- Fix FLUSHLEGACY to scan full keyspace and filter out !-prefixed keys, instead of scanning [nil, "!" which missed alphanumeric user keys - Simplify load() internal key detection to bytes.HasPrefix(key, !) to cover all internal namespaces (!lst|, !txn|, etc.) EOF )
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a migration to prefix Redis string keys with "!redis|str|" to distinguish them from internal metadata. It introduces a FLUSHLEGACY command for one-time cleanup of unprefixed keys and updates various operations (GET, SET, INCR, etc.) and the Lua context to handle the new prefixing. Review feedback identified issues in the key detection logic within the load and flushlegacy functions, noting that the current implementation is too broad and could lead to incorrect storage paths or accidental deletion of non-Redis data in shared environments.
| storageKey := key | ||
| userKey := extractRedisInternalUserKey(key) | ||
| if !bytes.HasPrefix(key, []byte("!")) { | ||
| storageKey = redisStrKey(key) | ||
| userKey = key | ||
| } |
There was a problem hiding this comment.
The logic to distinguish between user keys and internal keys in load is too broad. By checking only for the ! prefix, it incorrectly treats user keys that start with ! (e.g., !mykey) as internal keys, bypassing the !redis|str| prefixing. This leads to incorrect storage paths and missing TTL tracking for such keys. Additionally, it still allows potential collisions if a user key matches a known internal prefix (e.g., !redis|hash|...).
| storageKey := key | |
| userKey := extractRedisInternalUserKey(key) | |
| if !bytes.HasPrefix(key, []byte("!")) { | |
| storageKey = redisStrKey(key) | |
| userKey = key | |
| } | |
| storageKey := key | |
| userKey := extractRedisInternalUserKey(key) | |
| if userKey == nil && !isRedisTTLKey(key) && !bytes.HasPrefix(key, []byte("!lst|")) && !bytes.HasPrefix(key, redisTxnKeyPrefix) { | |
| storageKey = redisStrKey(key) | |
| userKey = key | |
| } |
References
- Avoid incomplete handling paths for entries to ensure they are preserved correctly and avoid state inconsistencies.
adapter/redis_compat_commands.go
Outdated
| if len(pair.Key) == 0 || pair.Key[0] != '!' { | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key}) | ||
| } |
There was a problem hiding this comment.
The flushlegacy command logic for identifying legacy keys is too simplistic. It skips any key starting with !, which means legacy bare keys that happen to start with ! will not be cleaned up. Furthermore, in a shared KV store environment, this command is extremely dangerous as it will delete any non-prefixed data belonging to other services (e.g., DynamoDB or S3 keys that don't start with !). The check should be more specific to known internal prefixes.
keyStr := string(pair.Key)
if len(pair.Key) == 0 || (!strings.HasPrefix(keyStr, "!redis|") && !strings.HasPrefix(keyStr, "!lst|") && !strings.HasPrefix(keyStr, kv.TxnKeyPrefix)) {
elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key})
}References
- Provide specific handling paths for entries to prevent data loss or state inconsistencies in shared environments.
- Replace bytes.HasPrefix(key, "!") in load() with isKnownInternalKey() that checks all known prefixes (!redis|, !lst|, !txn|, !ddb|, !s3|, !dist|) — user keys starting with "!" are now correctly prefixed - Use the same isKnownInternalKey() in FLUSHLEGACY to skip only keys belonging to known internal namespaces, not all "!"-prefixed keys
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a migration for Redis string keys by introducing a "!redis|str|" prefix and updates various commands to use this new scheme. It also adds a FLUSHLEGACY command for one-time cleanup of unprefixed keys. Feedback indicates that the current implementation is a breaking change because the load method lacks a fallback for existing bare keys. Additionally, FLUSHLEGACY leaves orphaned TTL keys, and the revised FLUSHALL command fails to clear legacy data, both of which could lead to state inconsistencies.
| if !isKnownInternalKey(key) { | ||
| storageKey = redisStrKey(key) | ||
| userKey = key | ||
| } |
There was a problem hiding this comment.
The migration to the !redis|str| prefix for string keys is a breaking change for existing data. The load method now only looks for the prefixed key, making existing bare string keys inaccessible. To avoid state inconsistencies and ensure data is preserved on disk, consider implementing a fallback mechanism to check for the bare key when the prefixed key is not found.
References
- Avoid silently dropping entries during serialization or normalization. If placeholder or incomplete entries are valid in some contexts, provide a specific, more lenient handling path for them to ensure they are preserved on disk and avoid state inconsistencies.
| elems := make([]*kv.Elem[kv.OP], 0, len(kvs)) | ||
| for _, pair := range kvs { | ||
| if !isKnownInternalKey(pair.Key) { | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key}) |
There was a problem hiding this comment.
FLUSHLEGACY only deletes the bare user keys but leaves behind their associated TTL keys (prefixed with !redis|ttl|). This leads to orphaned storage entries and state inconsistencies. Ensure that all associated metadata is handled to avoid leaving incomplete or orphaned entries in the store.
References
- Avoid silently dropping entries during serialization or normalization. If placeholder or incomplete entries are valid in some contexts, provide a specific, more lenient handling path for them to ensure they are preserved on disk and avoid state inconsistencies.
| {Op: kv.DelPrefix, Key: []byte("!redis|")}, | ||
| {Op: kv.DelPrefix, Key: []byte("!lst|")}, |
There was a problem hiding this comment.
Scoping FLUSHALL to !redis| and !lst| prefixes means that any remaining legacy bare keys will not be cleared, resulting in state inconsistencies. To maintain consistency, ensure that legacy entries are either handled or that the command's scope is clearly defined to prevent data remnants.
References
- Avoid silently dropping entries during serialization or normalization. If placeholder or incomplete entries are valid in some contexts, provide a specific, more lenient handling path for them to ensure they are preserved on disk and avoid state inconsistencies.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a migration for Redis string keys to use a new "!redis|str|" prefix, improving namespace isolation within the storage layer. It introduces a new FLUSHLEGACY command to clean up old unprefixed keys and updates various read/write paths—including transactions, Lua scripts, and standard commands—to support the new prefix while maintaining fallback compatibility for legacy data. The review feedback correctly identifies a bug in the transaction loading logic where keys could be double-prefixed, leading to failed lookups, and suggests correcting the return value of FLUSHLEGACY to reflect user-visible key counts rather than internal storage operations.
adapter/redis.go
Outdated
| if userKey != nil { | ||
| // For user string keys, use the fallback-aware reader. | ||
| val, _ = t.server.readRedisStringAt(key, t.startTS) | ||
| } else { | ||
| var err error | ||
| val, err = t.server.readValueAt(storageKey, t.startTS) | ||
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for loading string values in transactions has several issues: 1. It calls readRedisStringAt for all Redis internal keys (like hashes or sets) because userKey != nil is true for them, which is inefficient. 2. It passes the potentially prefixed key to readRedisStringAt, which will then attempt to prefix it again, leading to incorrect storage lookups (e.g., !redis|str|!redis|str|key). The condition should specifically check if the key is a Redis string (either bare or prefixed) and use the bare userKey for the fallback-aware read. Note that explicit conflict tracking for the bare key is not required per system architecture.
| if userKey != nil { | |
| // For user string keys, use the fallback-aware reader. | |
| val, _ = t.server.readRedisStringAt(key, t.startTS) | |
| } else { | |
| var err error | |
| val, err = t.server.readValueAt(storageKey, t.startTS) | |
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | |
| return nil, errors.WithStack(err) | |
| } | |
| } | |
| isString := !isKnownInternalKey(key) || bytes.HasPrefix(key, []byte(redisStrPrefix)) | |
| if isString { | |
| // For user string keys, use the fallback-aware reader with the bare key. | |
| val, _ = t.server.readRedisStringAt(userKey, t.startTS) | |
| } else { | |
| var err error | |
| val, err = t.server.readValueAt(storageKey, t.startTS) | |
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | |
| return nil, errors.WithStack(err) | |
| } | |
| } |
References
- Optimistic Concurrency Control (OCC) conflict checks are not required at the store layer because the leader is responsible for issuing consistent timestamps, which is expected to prevent write conflicts.
adapter/redis_compat_commands.go
Outdated
| conn.WriteError(err.Error()) | ||
| return | ||
| } | ||
| totalDeleted += len(elems) |
There was a problem hiding this comment.
totalDeleted is counting the number of internal storage operations (data key + TTL key) rather than the number of user-visible keys deleted. This makes the return value of FLUSHLEGACY inconsistent with standard Redis commands like DEL. It should count the number of legacy user keys found and processed.
| totalDeleted += len(elems) | |
| totalDeleted += len(elems) / 2 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a prefix migration for Redis string keys, introducing the !redis|str| prefix and a fallback mechanism to support legacy unprefixed keys. Key changes include the addition of a FLUSHLEGACY command for one-time cleanup, updates to core Redis operations to use the new prefixing logic, and modifications to FLUSHALL to target specific internal namespaces. Feedback focuses on ensuring transactional consistency by tracking legacy keys in read sets, correcting logic and error handling in the transaction load function, addressing semantic inconsistencies in FLUSHALL regarding legacy data, and suggesting throttling for the FLUSHLEGACY scan to prevent cluster instability.
| t.trackReadKey(storageKey) | ||
| if userKey != nil { | ||
| t.trackReadKey(redisTTLKey(userKey)) | ||
| } else if !bytes.HasPrefix(key, []byte(redisTTLPrefix)) { | ||
| t.trackReadKey(redisTTLKey(key)) | ||
| } |
There was a problem hiding this comment.
Consistency issue in load. When loading a bare user key that might require a fallback read from the unprefixed path, the bare key itself must be added to the transaction's read set. Without this, the transaction won't detect conflicts if the legacy key is modified or deleted by a concurrent transaction.
t.trackReadKey(storageKey)
if !isKnownInternalKey(key) {
t.trackReadKey(key)
}
if userKey != nil {
t.trackReadKey(redisTTLKey(userKey))
}References
- In transactional read-modify-write operations, ensure a consistent snapshot to prevent race conditions.
adapter/redis.go
Outdated
| if userKey != nil { | ||
| // For user string keys, use the fallback-aware reader. | ||
| val, _ = t.server.readRedisStringAt(key, t.startTS) | ||
| } else { | ||
| var err error | ||
| val, err = t.server.readValueAt(storageKey, t.startTS) | ||
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two issues here:
- Logic Error:
userKeyis non-nil for all internal Redis keys (e.g.,!redis|hash|...). CallingreadRedisStringAton these keys is incorrect as it will attempt a prefixed fallback. The fallback reader should only be used for bare user keys. - Error Handling: The error from
readRedisStringAtis silently ignored. Transient errors from the storage layer will result in the key being treated as non-existent, which can lead to data corruption during a read-modify-write cycle.
| if userKey != nil { | |
| // For user string keys, use the fallback-aware reader. | |
| val, _ = t.server.readRedisStringAt(key, t.startTS) | |
| } else { | |
| var err error | |
| val, err = t.server.readValueAt(storageKey, t.startTS) | |
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | |
| return nil, errors.WithStack(err) | |
| } | |
| } | |
| if !isKnownInternalKey(key) { | |
| // For bare user string keys, use the fallback-aware reader. | |
| var err error | |
| val, err = t.server.readRedisStringAt(key, t.startTS) | |
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | |
| return nil, errors.WithStack(err) | |
| } | |
| } else { | |
| var err error | |
| val, err = t.server.readValueAt(storageKey, t.startTS) | |
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | |
| return nil, errors.WithStack(err) | |
| } | |
| } |
References
- Avoid silently dropping entries or ignoring errors to ensure data is preserved and avoid state inconsistencies.
| {Op: kv.DelPrefix, Key: []byte("!redis|")}, | ||
| {Op: kv.DelPrefix, Key: []byte("!lst|")}, |
There was a problem hiding this comment.
Scoping FLUSHALL to only !redis| and !lst| prefixes is problematic because it leaves legacy bare string keys untouched. This breaks the expected semantics of FLUSHALL (clearing all Redis data) and creates a data integrity risk: if a prefixed key is deleted by FLUSHALL but its legacy bare counterpart remains, the old value will "reappear" on the next GET. FLUSHALL should be updated to also clear the bare keyspace.
References
- Ensure all relevant entries are handled during operations to avoid state inconsistencies.
| redisStrKey(key), | ||
| redisTTLKey(key), |
There was a problem hiding this comment.
In trackTypeReadKeys, the bare key should also be tracked to ensure snapshot isolation when checking the type of legacy keys. If only the prefixed key is tracked, concurrent modifications to the legacy bare key by other transactions will not be detected, potentially leading to inconsistent typing decisions.
redisStrKey(key),
key,
redisTTLKey(key),References
- Avoid silently dropping entries or incomplete handling to ensure they are preserved and avoid state inconsistencies.
| // regardless of their byte range. | ||
| var cursor []byte | ||
| for { | ||
| kvs, err := r.store.ScanAt(ctx, cursor, nil, batchSize, readTS) |
There was a problem hiding this comment.
The flushlegacy command performs a full keyspace scan and dispatches deletions in batches without any throttling. On a large production dataset, this could saturate the Raft log and cause significant latency spikes or even instability for other cluster operations. Consider implementing a mechanism to throttle the deletion rate or adding a small sleep between batches.
References
- Apply bounds to operations that can grow from external requests to prevent resource exhaustion and system instability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a prefix migration for Redis string keys, introducing the !redis|str| prefix and a FLUSHLEGACY command for cleanup. The changes update read and write operations to support both new prefixed keys and legacy bare keys. Feedback focuses on ensuring transaction consistency by tracking both key types in trackTypeReadKeys and load to prevent write skew, and correctly handling errors when reading string values during transactions.
| redisStreamKey(key), | ||
| redisHLLKey(key), | ||
| key, | ||
| redisStrKey(key), |
There was a problem hiding this comment.
The trackTypeReadKeys function is now missing the bare key which is still used as a fallback for legacy string data in rawKeyTypeAt. To ensure correct conflict detection and snapshot consistency during transactions, both the prefixed and bare keys must be tracked.
| redisStrKey(key), | |
| redisStrKey(key), | |
| key, |
References
- Avoid silently dropping entries or metadata to ensure they are preserved on disk and avoid state inconsistencies.
| t.trackReadKey(storageKey) | ||
| if userKey != nil { | ||
| t.trackReadKey(redisTTLKey(userKey)) | ||
| } else if !bytes.HasPrefix(key, []byte(redisTTLPrefix)) { | ||
| t.trackReadKey(redisTTLKey(key)) | ||
| } | ||
| tv := &txnValue{} | ||
| val, err := t.server.readValueAt(key, t.startTS) | ||
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | ||
| return nil, errors.WithStack(err) | ||
| var val []byte | ||
| isString := !isKnownInternalKey(key) || bytes.HasPrefix(key, []byte(redisStrPrefix)) | ||
| if isString { | ||
| // For user string keys, use the fallback-aware reader with the bare key. | ||
| val, _ = t.server.readRedisStringAt(userKey, t.startTS) | ||
| } else { | ||
| var err error | ||
| val, err = t.server.readValueAt(storageKey, t.startTS) | ||
| if err != nil && !errors.Is(err, store.ErrKeyNotFound) { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
When loading a potential string key, the transaction must track the bare userKey in addition to the storageKey. Since readRedisStringAt falls back to reading the bare key if the prefixed one is missing, failing to track it means concurrent modifications to legacy keys won't be detected, leading to write skew or lost updates. Additionally, errors from readRedisStringAt should be handled and propagated to avoid inconsistent state.
t.trackReadKey(storageKey)
isString := !isKnownInternalKey(key) || bytes.HasPrefix(key, []byte(redisStrPrefix))
if userKey != nil {
t.trackReadKey(redisTTLKey(userKey))
if isString && !bytes.Equal(storageKey, userKey) {
t.trackReadKey(userKey)
}
}
tv := &txnValue{}
var val []byte
if isString {
// For user string keys, use the fallback-aware reader with the bare key.
var err error
val, err = t.server.readRedisStringAt(userKey, t.startTS)
if err != nil && !errors.Is(err, store.ErrKeyNotFound) {
return nil, errors.WithStack(err)
}
} else {
var err error
val, err = t.server.readValueAt(storageKey, t.startTS)
if err != nil && !errors.Is(err, store.ErrKeyNotFound) {
return nil, errors.WithStack(err)
}
}References
- Avoid silently dropping entries to ensure they are preserved on disk and avoid state inconsistencies.
- In transactional read-modify-write operations, set the transaction's start timestamp to the read timestamp to ensure a consistent snapshot and prevent race conditions.
…legacy keys in FLUSHALL
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a prefix migration for Redis string keys, introducing the "!redis|str|" namespace and updating read/write paths to support both prefixed and legacy bare keys. It adds a FLUSHLEGACY command for one-time cleanup and integrates the new prefixes into standard flush operations. Review feedback identifies a potential inconsistency in key normalization within the transaction context compared to other write paths, concerns regarding timestamp consistency in transactional operations, and recommends implementing leader proxying for the FLUSHLEGACY command to maintain consistency with existing write operations.
| func (t *txnContext) load(key []byte) (*txnValue, error) { | ||
| k := string(key) | ||
| // If the key is already an internal key (e.g., !redis|hash|..., | ||
| // !lst|..., !txn|..., !ddb|..., !s3|..., !dist|...), use it as-is. | ||
| // Otherwise, it's a bare user key for a string value — prefix it. | ||
| storageKey := key | ||
| userKey := extractRedisInternalUserKey(key) | ||
| if !isKnownInternalKey(key) { | ||
| storageKey = redisStrKey(key) | ||
| userKey = key | ||
| } |
There was a problem hiding this comment.
The logic in txnContext.load to automatically prefix keys based on isKnownInternalKey creates an inconsistency between transactional and non-transactional paths. While load skips prefixing if the key already looks like an internal key, other paths like saveString and incr always apply the prefix. This inconsistency can lead to data silos and collisions within the transaction context. To maintain state consistency, ensure that entries are not silently dropped or misrouted during normalization. Additionally, for these transactional read-modify-write operations, the transaction's start timestamp must be set to the read timestamp and acquired after the linearizable read fence to guarantee a consistent snapshot view.
References
- Avoid silently dropping or misrouting entries during normalization to ensure they are preserved on disk and avoid state inconsistencies.
- In transactional read-modify-write operations, set the transaction's start timestamp to the read timestamp to ensure a consistent snapshot and prevent race conditions.
- When implementing linearizable reads in an MVCC system, ensure that the timestamp used for the MVCC read is acquired after the linearizable read fence has completed to guarantee a consistent snapshot view.
| if !r.coordinator.IsLeader() { | ||
| conn.WriteError("ERR FLUSHLEGACY must be run on the leader") | ||
| return | ||
| } |
There was a problem hiding this comment.
The flushlegacy command should proxy the request to the leader if the current node is a follower, consistent with FLUSHALL and other write operations. When implementing this proxying logic, ensure that leadership status is verified after acquiring the lock that protects the operation to prevent race conditions and ensure the operation is handled by the correct node.
References
- To prevent race conditions in read-modify-write operations that require leadership, verify leadership status after acquiring the lock that protects the operation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a migration for Redis string keys by introducing a new !redis|str| prefix. It includes a fallback mechanism in readRedisStringAt and txnContext.load to support reading legacy unprefixed keys, and adds a FLUSHLEGACY command for manual cleanup. Additionally, FLUSHALL has been updated to target specific Redis prefixes and perform a scan for legacy keys. Review feedback identifies critical concerns regarding the completeness of the legacy scan in sharded environments, a performance regression in FLUSHALL due to the full keyspace scan, and a potential issue where legacy keys are not explicitly deleted when overwritten within a transaction.
adapter/redis_compat_commands.go
Outdated
| // deleted. If deleteTTL is true, also deletes associated TTL keys for each | ||
| // legacy key (when called standalone); callers that already cleared TTL keys | ||
| // via DEL_PREFIX should pass false. | ||
| func (r *RedisServer) deleteLegacyKeys(ctx context.Context, readTS uint64, deleteTTL bool) (int, error) { |
There was a problem hiding this comment.
In a sharded environment, r.store.ScanAt typically only scans the local shard or the shard managed by the default group (when using LeaderRoutedStore). Since legacy unprefixed keys are distributed across all shards based on their key value, this scan will miss legacy keys on all other shards. This makes FLUSHALL and FLUSHLEGACY incomplete in sharded deployments. A cluster-wide cleanup would require coordinating scans across all shard leaders.
adapter/redis_compat_commands.go
Outdated
| // Also clean up any legacy bare keys so they don't "reappear" via the | ||
| // fallback read path. TTL keys are already deleted by the !redis| prefix | ||
| // delete above, so pass deleteTTL=false. | ||
| if _, err := r.deleteLegacyKeys(ctx, r.readTS(), false); err != nil { |
There was a problem hiding this comment.
FLUSHALL (via flushDatabase) now performs a full keyspace scan using deleteLegacyKeys to remove unprefixed keys. This is a significant performance regression from the previous DelPrefix(nil) implementation, especially for large datasets in other namespaces (e.g., DynamoDB). Since FLUSHLEGACY is provided for one-time migration, FLUSHALL should ideally be scoped to Redis prefixes only, or the legacy cleanup should be made optional to maintain the expected performance of flush operations.
| func (t *txnContext) load(key []byte) (*txnValue, error) { | ||
| k := string(key) | ||
| // If the key is already an internal key (e.g., !redis|hash|..., | ||
| // !lst|..., !txn|..., !ddb|..., !s3|..., !dist|...), use it as-is. | ||
| // Otherwise, it's a bare user key for a string value — prefix it. | ||
| storageKey := key | ||
| userKey := extractRedisInternalUserKey(key) | ||
| if !isKnownInternalKey(key) { | ||
| storageKey = redisStrKey(key) | ||
| userKey = key | ||
| } | ||
| k := string(storageKey) |
There was a problem hiding this comment.
Mapping bare user keys to prefixed storage keys in the t.working map enables fallback reads but prevents the transaction from ever explicitly addressing or deleting the bare legacy key. Consequently, legacy keys will persist in the store even after being logically overwritten or deleted within a transaction (e.g., via SET or DEL), leading to stale data remaining until a manual FLUSHLEGACY is performed. To avoid state inconsistencies and ensure that entries are correctly handled during normalization/migration, consider explicitly marking the bare key for deletion in t.working when a legacy string is encountered.
References
- Avoid silently dropping entries during serialization or normalization to ensure they are preserved on disk and avoid state inconsistencies.
…sion FLUSHALL now only deletes prefixed Redis keys via DEL_PREFIX. Legacy bare keys should be cleaned up separately via FLUSHLEGACY.
|
@claude review |
[Consistency] レビュー結果PR #486 — feat: add 軽微な問題が 3 点あります。 1. FLUSHALL 後のレガシーベアキー残存（設計上の意図的決定だが運用リスク）
2. FLUSHLEGACY 実行後の TTL キー孤立
レガシーキーに TTL が設定されていた場合、FLUSHLEGACY 後に TTL キーだけが残存します。 データキーが存在しなければ GET は null を返すため読み取り結果への影響は限定的です。ただし、後から 3. FLUSHLEGACY の非トランザクション batch dispatch
|
[Performance] レビュー結果P1: ホットパスでのダブルストレージ読み取り** — ** func (r *RedisServer) readRedisStringAt(key []byte, readTS uint64) ([]byte, error) {
v, err := r.readValueAt(redisStrKey(key), readTS)
if err == nil {
return v, nil
}
if !errors.Is(err, store.ErrKeyNotFound) {
return nil, err
}
return r.readValueAt(key, readTS) // ← 2回目のストレージ読み取り
}GET / GETDEL / INCR / TYPE チェックなど、すべての文字列読み取りパスがこの関数を通るようになった。 FLUSHLEGACY 実行後にフォールバックパスを削除する計画・期限を明示するか、完了まで監視するメトリクスを設けることを推奨する。 P2: トランザクション競合追跡セットの肥大化** — ** // 変更前: key を1つだけ追跡
key,
// 変更後: prefixed + bare の2キーを追跡
redisStrKey(key),
key, // legacy bare key for fallback reads文字列型の TYPE チェック時にリードセットへ登録するキー数が1→2に増加した。トランザクションごとに扱うリードセットのマップエントリが増え、競合検出のメモリ消費とスキャンコストが増加する。 P3: での不要な3アロケーション** — ** // 変更後
tv, ok := t.working[string(redisStrKey([]byte(key)))]
// ^^^^^^^^^^^^^^^^^^^^^^^^^
// 1. []byte(key) → ヒープアロケーション
// 2. redisStrKey(...) → append で新スライス
// 3. string(...) → 文字列コピーはすでに 型のため、以下のように文字列連結1回に置き換えられる： tv, ok := t.working[redisStrPrefix+key]これにより3アロケーションが0（コンパイラが定数+変数連結を最適化）または1に削減される。 P4: の DEL ごとの余分なアロケーション** — ** bareK := string(key)
if _, ok := t.working[bareK]; !ok {
t.working[bareK] = &txnValue{} // ← DEL ごとに必ずアロケーション
}
t.working[bareK].deleted = true
t.working[bareK].dirty = truelegacy bare key が working map にない場合、毎回 をアロケートして挿入する。 まとめ
P1・P2 はマイグレーション期間の意図的なトレードオフと理解できるが、FLUSHLEGACY 完了後に確実にフォールバックコードを除去するためのチケット・TODO を残すことを強く推奨する。P3・P4 は即修正可能。 |
[Data Loss] レビュー結果確認事項✅ 問題なし:
|
[Data Loss] レビュー結果対象: PR #486 - feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL変更の概要Redis 文字列キーに 指摘事項[High] FLUSHALL がレガシー素キーを削除しない → 見かけ上の削除後にデータが残存場所: 変更前は 問題シナリオ:
推奨: [Medium]
|
[Consistency] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL 🔴 Issue 1: FLUSHALL/FLUSHDB がレガシー素キーを削除しない（意味論的不整合）場所: 問題: 変更前の // 変更後
{Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
// レガシーな素キー（例: "mykey"）は削除されないRedis の仕様では
Jepsen などのテストで 推奨: 🟡 Issue 2:
|
[Consistency] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL指摘事項[MEDIUM] FLUSHALL のセマンティクス変更によるファントムデータリスク
FLUSHALL 後に FLUSHLEGACY を実行しないと裸キーが残り、同名キーへの GET で旧データが返る可能性がある。この2段階操作は強制されておらず、利用者が知らずに FLUSHALL だけ実行するケースで一貫性が損なわれる。 提案: FLUSHALL 内で FLUSHLEGACY 相当の処理を自動実行するか、または FLUSHALL 時に裸キーが残っている場合は警告を返す。 [MEDIUM] deleteLegacyKeys のバッチ間競合
マイグレーション完了後に新規裸キーが書かれることは想定外だが、移行期間中（移行途中のノードが混在するクラスタ）は発生し得る。 提案: FLUSHLEGACY はリーダー昇格直後ではなく、全ノードのマイグレーション完了を確認後に実行する運用ガイドを明記する。 [LOW] stageKeyDeletion が未ロードの裸キーを working に追加 // adapter/redis.go
if _, ok := t.working[bareK]; !ok {
t.working[bareK] = &txnValue{}
}
t.working[bareK].deleted = true
t.working[bareK].dirty = true
[確認済み - 問題なし]
|
[Data Loss] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL要注意: FLUSHALL のセマンティクス変更によるデータ残留リスク 問題1 (HIGH): FLUSHALL がレガシー bare キーを削除しない変更前: {Op: kv.DelPrefix, Key: nil} // 全キーを削除変更後: {Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
{Op: kv.DelPrefix, Key: []byte("!zs|")},リスク:
コメントに「FLUSHLEGACY を先に実行せよ」とあるが、オペレーターが知らずに FLUSHALL だけ実行した場合にレガシーデータが消えないことを ドキュメント・エラーメッセージ レベルで明示する仕組みがない点が懸念です。 問題2 (LOW): FLUSHLEGACY の TTL キー残留による将来キーへの誤 expire
シナリオ:
その他 (問題なし)
|
[Performance] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL1.
|
[Data Loss] レビュー結果対象PR: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL 問題1（重要）: FLUSHALL後にレガシーベアキーが読み出せる — スタールデータの露出パス:
再現シナリオ:
FLUSHALL 後にデータベースがクリアされたと見なしたアプリケーションが、フラッシュ前のデータを読み取ってしまいます。 対処案: FLUSHALL の前に FLUSHLEGACY の実行をドキュメントで必須要件として明記するか、FLUSHALL 自体がレガシーキー削除を含むよう (FLUSHLEGACY を内部呼び出し) 実装を検討してください。 問題2（軽微）: FLUSHLEGACY がバッチ間で非アトミックパス: 削除は 1000 件ごとのバッチに分割され、各バッチが個別の Raft ログエントリとして発行されます。サーバークラッシュやタイムアウト発生時、バッチの途中で停止するとレガシーキーが一部削除・一部残存する中間状態になります。 FLUSHLEGACY 自体は冪等な操作（再実行可能）なので真のデータロストにはなりませんが、中間状態の期間に「問題1」のスタールデータ露出が継続します。進捗や完了状態をクライアントへ明示する仕組みがあると安全です。 確認済み（問題なし）
|
[Concurrency] レビュー結果対象PR: #486 問題点[MEDIUM] FLUSHALL がレガシーベアキーを削除しない（データ残留）場所: // Before: nil（全キー削除）
{Op: kv.DelPrefix, Key: nil}
// After: 既知プレフィックスのみ削除
{Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
{Op: kv.DelPrefix, Key: []byte("!zs|")},FLUSHLEGACY を実行していない環境で FLUSHALL を呼ぶと、レガシー裸キー（ コードコメントに「Run FLUSHLEGACY first」とあるが、実運用でこの手順漏れは起きやすい。起動時の自動マイグレーション、または FLUSHALL 時に FLUSHLEGACY を内包する処理を検討されたい。 [LOW]
|
[Consistency] レビュー結果対象PR: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL 問題あり (2件)問題1 (重大): FLUSHALL がレガシー裸キーを削除しないため、フラッシュ後も GET でデータが返る箇所: 概要: FLUSHALL は 一方、 結果として、FLUSHLEGACY を先に実行しないまま FLUSHALL を実行した場合:
これは FLUSHALL が成功したにも関わらず古いデータが読めてしまう という線形化可能性の違反です。 影響: FLUSHLEGACY を先行実行しないオペレーターが FLUSHALL でデータが消えたと思いつつ、実際にはレガシーキーが残り続けるリスクがある。 提案: FLUSHALL のコメントやドキュメントにより強い警告を加えるか、FLUSHALL 内で 問題2 (軽微): INCR がレガシー裸キーをクリーンアップしない箇所: 概要: INCR はレガシー裸キーから値を読み取り (
提案: INCR の書き込み時に裸キーの その他 (問題なし)
|
[Test Coverage] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL概要Redis文字列キーに カバーされているケース ✅
不足しているテストケース ❌1.
func TestReadRedisStringAtFallsBackToLegacyBareKey(t *testing.T) {
st := store.NewMVCCStore()
// レガシー: bare キーに書き込む
_ = st.PutAt(ctx, []byte("mykey"), []byte("v1"), 1, 0)
srv := newTestRedisServer(st)
val, err := srv.readRedisStringAt([]byte("mykey"), st.LastCommitTS())
require.NoError(t, err)
require.Equal(t, []byte("v1"), val) // フォールバック経由で取得できること
}2. 新コマンド 提案するテストケース: func TestFlushLegacyDeletesBareKeysPreservingPrefixedKeys(t *testing.T) {
// セットアップ: bare key と prefixed key を両方書き込む
// FLUSHLEGACY を実行
// bare key が削除されていることを確認
// !redis|str|mykey は残っていることを確認
// !redis|ttl|mykey (TTL キー) は残っていることを確認
}
func TestFlushLegacyBatchBoundary(t *testing.T) {
// batchSize(1000)を超えるレガシーキーを書き込んで、複数バッチが正しく処理されることを確認
}3. FLUSHALL が nil (全消去) から func TestFlushAllDoesNotDeleteNonRedisNamespaces(t *testing.T) {
// !txn|, !ddb|, !s3| など他ネームスペースのキーを書き込む
// FLUSHALL を実行
// Redisキー (!redis|str|, !lst|, !zs|) は削除されていること
// 他ネームスペースのキーが残っていることを確認
}4.
5.
6. レガシー→新フォーマットの共存シナリオ（重要度: 高） このシナリオ全体のテストが欠落している。 判定: 重大な不足あり FLUSHLEGACY と FLUSHALL の動作変更・readRedisStringAt のフォールバックはデータ損失リスクに直結するため、テストを追加してからマージを推奨する。 |
[Performance] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL[P1]
|
[Data Loss] レビュー結果PR: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL 問題あり[HIGH] FLUSHLEGACY が並行書き込みとのコンフリクト検出なしに削除を実行する
if err := r.dispatchElems(ctx, false, readTS, elems); err != nil {
return totalDeleted, err
}
データロストシナリオ：
推奨対応： バッチ毎に [MEDIUM] FLUSHALL がlegacyベアキーを削除しなくなった（動作変更）変更後のFLUSHALLは コード内コメントに記述されているが、ユーザーが「FLUSHALL後に全データが削除された」と認識して後続の処理を行うと、残存するlegacyキーが予期せず参照される可能性がある。FLUSHALLの前にFLUSHLEGACYの実行を義務付けるガードや警告が望ましい。 [LOW] DEL操作ごとにベアキーのtombstoneが不要に書かれる
|
[Consistency] レビュー結果対象: 問題1（中）: FLUSHALL がレガシーベアキーを削除しない — データ残留リスク// adapter/redis_compat_commands.go
_, err := r.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
Elems: []*kv.Elem[kv.OP]{
{Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
{Op: kv.DelPrefix, Key: []byte("!zs|")},
},
})変更前の FLUSHALL は 懸念: アプリケーションが「FLUSHALL で全データが消える」という前提で動作している場合（テスト環境の初期化、隔離など）、レガシーデータが混入したまま後続処理が走りうる。コードコメントには注記がありますが、強制メカニズムがないため運用上の事故が起きやすい。 推奨対策: FLUSHALL 実行時にレガシーキーが存在するかどうかを一度チェックし、存在する場合は警告レスポンス（または自動的に FLUSHLEGACY を先行実行）する仕組みを検討してください。 問題2（低）:
|
[Concurrency] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL問題が 1 件、注意点が 1 件 あります。 [問題あり]
|
[Test Coverage] レビュー結果対象PR: #486 概要このPRは大きなストレージ移行を含む変更です。新しいキープレフィックス ( 問題1:
|
| カテゴリ | 状況 |
|---|---|
isKnownInternalKey() ユニットテスト |
なし |
文字列キー移行テスト (redis_storage_migration_test.go) |
なし |
FLUSHLEGACY コマンドテスト |
なし |
スコープ変更後の FLUSHALL テスト |
なし |
DEL によるレガシーキー削除テスト |
なし |
| 既存テストの破損 | なし（更新済み） |
移行ロジックの正確性はデータの永続性に直結するため、特に readRedisStringAt フォールバック・FLUSHLEGACY・FLUSHALL スコープのテストを追加することを強くお勧めします。
[Test Coverage] レビュー結果PR #486「feat: add 概要テスト変更は 不足しているテスト1.
|
| 対象 | テスト有無 |
|---|---|
readRedisStringAt() fallback |
❌ なし |
isKnownInternalKey() |
❌ なし |
flushlegacy コマンド |
❌ なし |
deleteLegacyKeys() |
❌ なし |
flushall スコープ変更 |
❌ なし |
| legacy key migration 互換性 | ❌ なし |
txnContext の変更 |
❌ なし |
| Lua context の変更 | ❌ なし |
| 既存テストの修正 | ✅ 3件修正済み |
新機能・変更ロジックのほとんどにテストが存在しません。特に readRedisStringAt() のfallback動作、flushlegacy コマンド、flushall のスコープ変更は本番環境でのデータ損失リスクがあるため、優先的にテストを追加することを推奨します。
[Performance] レビュー結果1. レガシーキーへのダブルリード【中】対象:
2.
|
[Data Loss] レビュー結果PR #486:
|
[Data Loss] レビュー結果PR #486:
|
| # | リスク | 内容 |
|---|---|---|
| 1 | 中 | FLUSHALL 後もレガシーデータが読み取れる (FLUSHALL の不完全なフラッシュ) |
| 2 | 低 | FLUSHLEGACY のローリングアップグレード中の競合なし削除 |
[Data Loss] レビュー結果対象PR: #486 🔴 問題1: FLUSHALL がレガシーベアキーを削除しない（中〜高リスク）場所: // 変更前
{Op: kv.DelPrefix, Key: nil} // 全データ削除
// 変更後
{Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
{Op: kv.DelPrefix, Key: []byte("!zs|")},
FLUSHLEGACY の事前実行が前提だが、強制する仕組みがなく、コマンドの存在をユーザーが知らなければ見落とす。 提案: FLUSHALL 実行時にレガシーキーが存在すれば警告を返す、またはドキュメント/RESP エラーメッセージで FLUSHLEGACY が必要な旨を明示する。 🟡 問題2: INCR/INCRBY がレガシーベアキーを削除しない（低〜中リスク）場所: return r.dispatchElems(ctx, true, readTS, []*kv.Elem[kv.OP]{
{Op: kv.Put, Key: redisStrKey(cmd.Args[1]), Value: ...},
{Op: kv.Del, Key: redisTTLKey(cmd.Args[1])},
// ← レガシーベアキー (cmd.Args[1]) の Del がない
})INCR はレガシーベアキーから値を読み取り ( 一方、SET は SET・DEL 系コマンドとの挙動の不整合であり、統一が望ましい。 🟡 問題3: FLUSHLEGACY が TTL キーを削除しない（低リスク）場所: // TTL keys are intentionally NOT deleted...レガシーベアキーに TTL が設定されていた場合、FLUSHLEGACY はデータキー（ベア 通常、次回の 総評: FLUSHALL の挙動変更（問題1）が最も影響範囲が広く、既存クライアントが FLUSHALL で全データクリアを期待している場合に意図しないデータ残存を引き起こすリスクがある。レガシーキーの削除漏れ（問題2）も INCR 多用ケースで徐々にストレージが汚染される点で注意が必要。 |
[Performance] レビュー結果対象PR: #486
|
[Consistency] レビュー結果PR #486: feat: add `!redis|str|` prefix to Redis string keys and scope FLUSHALL🔴 高: FLUSHALL のセマンティクス変更による一貫性リグレッション場所: 変更前: 変更後:
PR のコメントに「FLUSHLEGACY を先に実行せよ」と書かれていますが、この前提条件はクライアントから不可視であり、FLUSHALL の線形化可能性（Linearizability）契約を破ります。 提案: FLUSHALL 内でも 🟡 中: FLUSHLEGACY の非トランザクショナル dispatch によるデータ消失リスク場所: ```go
具体的なシナリオ：
また、 🟡 中:
|
[Consistency] レビュー結果問題1 [HIGH] FLUSHALL/FLUSHDB が legacy bare key を削除せず、線形化可能性が破れるファイル: 変更前は // After
{Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
{Op: kv.DelPrefix, Key: []byte("!zs|")},移行前に書き込まれた legacy bare key（プレフィックスなしキー）はこの操作に含まれないため、FLUSHALL/FLUSHDB 完了後も GET で値が返る状態が発生する。 線形化可能性の問題:
影響範囲: FLUSHDB も同じ 問題2 [LOW]
|
[Concurrency] レビュー結果対象: PR #486 問題 1 [中] FLUSHLEGACY の同時実行ガードがない
// flushlegacy (redis_compat_commands.go)
ctx, cancel := context.WithTimeout(context.Background(), redisFlushLegacyTimeout)
defer cancel()
totalDeleted, err := r.deleteLegacyKeys(ctx, r.readTS())2つのゴルーチンが同じ 問題 2 [中]
|
[Consistency] レビュー結果PR #486: feat: add !redis|str| prefix to Redis string keys and scope FLUSHALL
|
1 participant
Summary
!redis|str|prefix to all Redis string key storage paths, making them distinguishable from DynamoDB, S3, and other namespaces!redis|*and!lst|*), preventing accidental deletion of DynamoDB/S3/distribution dataFLUSHLEGACYcommand to scan and delete old unprefixed string keys written before the migrationTest plan
go test ./...)