26 enhanced kv store key value experience#80
Conversation
|
/crush_fast AI review started. |
| if (!def || typeof def !== 'object') { | ||
| throw new ChelErrorKvSlotInvalid('[chelonia/kv] defineSlot: invalid definition') | ||
| } | ||
| if (typeof def.key !== 'string' || def.key.length === 0) { | ||
| throw new ChelErrorKvSlotInvalid('[chelonia/kv] defineSlot: invalid key') | ||
| } | ||
| const types = Array.isArray(def.contractType) ? def.contractType : [def.contractType] | ||
| if (types.length === 0) { | ||
| throw new ChelErrorKvSlotInvalid('[chelonia/kv] defineSlot: contractType required') | ||
| } | ||
| // Runtime validation of optional fields (SBP selectors are callable | ||
| // from JavaScript without TypeScript enforcement). | ||
| if (def.match != null && typeof def.match !== 'function') { | ||
| throw new ChelErrorKvSlotInvalid('[chelonia/kv] defineSlot: match must be a function') | ||
| } |
There was a problem hiding this comment.
Consider using Zod for schema validation.
|
Testing this, I noticed this for Increasing the FIFO queue size from 8 to 128 seems to fix the failing tests, but we shouldn't be having so many conflicts writing to the notifications queue, and this points to a likely bug in the echo-suppression logic this PR implements. If we indeed have buggy logic, it needs to be fixed (to determine this, we need to examine actual update calls to see what's being written). The logs suggest that the client is confused about what should be written / updated and attempts to repeatedly make the same update. This PR introduces a nonce in KV writes (which are then put in a FIFO queue) so that when we receive a value over pubsub, we can tell whether it's the value we've just written or an update made by someone else. However, we likely don't need this additional nonce (which is stored in the KV value) since we already hash KV values and use the
This seems resolved now, an it was caused by |
| this.kvSlots = new Map() | ||
| this.kvSlotsByContractID = new Map() | ||
| this.kvActiveFilters = new Map() | ||
| this.kvFilterDirty = new Set() | ||
| this.kvLocalEchoCIDs = new Map() | ||
| this.kvPendingWrites = new Map() | ||
| this.defContractKvByManifest = new Map() |
There was a problem hiding this comment.
All of the other variables in this section begin with kv, kinda indicating that they're together as part of the same system, except for this last one, which also has Kv in the name but it's not at the start of the variable name. For consistency, would it make sense to move that to the start of the variable name, e.g. this.kvDefConractByManifest, or is it intended to be set apart from the others?
There was a problem hiding this comment.
It's different from the others in the sense that it's only used in / by 'chelonia/defineContract', which mostly follows that specific naming convention (this.defContractSelectors, this.defContract).
OTOH, you're right that we do have all of these other keys starting with kv when they are KV-related.
| // Share one lazy parsed wrapper between the legacy callback and the | ||
| // slot layer. If either consumer forces `.data`, the decoded value | ||
| // or thrown error is cached; both consumers can therefore observe | ||
| // and log the same decode failure, and the legacy callback may | ||
| // decode frames the slot layer would skip as self-echoes. | ||
| const parsed = parseEncryptedOrUnencryptedMessage<object>(this, { | ||
| contractID: msg.channelID, | ||
| meta: msg.key, | ||
| serializedData: JSON.parse(Buffer.from(msg.data).toString()) | ||
| }) | ||
| if (legacyKvHandler) { | ||
| try { | ||
| ;( | ||
| legacyKvHandler as unknown as ( | ||
| this: PubSubClient, | ||
| msg: [string, ParsedEncryptedOrUnencryptedMessage<object>], | ||
| ) => void | ||
| ).call(this.pubsub, [msg.key, parsed]) | ||
| } catch (e) { | ||
| console.error( | ||
| `[chelonia] legacy kv pubsub callback threw for ${msg.channelID}::${msg.key}`, | ||
| e | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't quite understand what this "legacy layer" thing is. Do we need it or can it be removed?
There was a problem hiding this comment.
Whether we 'need' it or not depends on what the API should look like. This is used for supporting the existing way of handling KV updates in Group Income which doesn't use chelonia/kv/update.
If we were to remove it, then application developers won't have easy access to these lower level primitives to handle KV updates directly.
There was a problem hiding this comment.
Letting developers have access to low-level APIs and doing backwards compatibility when it's not needed are two different things.
The word "legacy" sounds like backwards compatibility. If it's not backwards compat then it should probably be renamed. If it is backwards compat then it should be removed.
2 participants
Closes #26
KV-REVAMPED.mdKV-REVAMPED.mdKV-REVAMPED.md contains the spec used (which this PR should conform to).
AI disclosure
GPT-5.5, Opus 4.7 and GLM 5.1 were used in generating code for this PR. GPT-5.5, Opus 4.7 & GLM 5.2 were used for reviews.