Node: add Batches support #3702
Conversation
shohamazon
added
node 🐢
node/src/BaseClient.ts
Outdated
| raiseOnError, | ||
| options as ClusterBatchOptions, | ||
| ); | ||
| }).then((result: T) => { |
There was a problem hiding this comment.
Find a way to avoid an extra branch for each command, and limit it to one if
node/src/BaseClient.ts
Outdated
| ); | ||
| }).then((result: T) => { | ||
| if (Array.isArray(command) && Array.isArray(result)) { | ||
| for (const item of result) { |
There was a problem hiding this comment.
Using for of, create a generator before starting looping and then get the object filed every time from the object.
Instead, use a simple loop of for let i etc., and cache the length outside the loop to avoid getting into the length every time.
e.g.
const loopLen = result.length;
for(let i = 0; i < loopLen; i++)
| // batch.configSet({[maxmemoryPolicyKey]: "allkeys-random"}); | ||
| expect(response[0]).toEqual("OK"); | ||
| // transaction.set(key, "foo"); | ||
| // batch.set(key, "foo"); |
There was a problem hiding this comment.
The comments are to know what is the content of the commands? We don't have a maintainable way to do it?
Who is going to know where to look for when there's a bug? Maybe a doc link to the test location?
There was a problem hiding this comment.
I just renamed transaction to batch
This is just an easy way to know the response for each command
|
|
||
| try { | ||
| const pipeline = new ClusterBatch(false); | ||
| pipeline.set("abc", "value"); |
There was a problem hiding this comment.
add comment of knowing its different slots and how
| async (protocol, isAtomic) => { | ||
| const client = await GlideClusterClient.createClient( | ||
| getClientConfigurationOption(cluster.getAddresses(), protocol, { | ||
| requestTimeout: 2000, |
There was a problem hiding this comment.
Do we recommend in the docs somewhere to use high timeout for batching? the docs of the config, not of the batch
There was a problem hiding this comment.
Not that I am aware of
There was a problem hiding this comment.
can we do? its seems that you run tests with pretty much sure knowledge
There was a problem hiding this comment.
Not really, I just copied the same structure above
|
|
||
| try { | ||
| expect(await client.configResetStat()).toEqual("OK"); | ||
| const key = uuidv4(); |
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
--------- Signed-off-by: Shoham Elias <shohame@amazon.com>
Issue link
This Pull Request is linked to issue (URL): #3546
Checklist
Before submitting the PR make sure the following are checked: