denoland / deploy_feedback

For reporting issues with Deno Deploy
https://deno.com/deploy
74 stars 5 forks source link

[KV Feedback]: Request for additional logic functionality for KV atomic(). #398

Closed Octo8080X closed 1 year ago

Octo8080X commented 1 year ago

🔍

Type of feedback

Feature request

Description

I have a request for KV atomic().

For example, if a key is not registered, a new key is registered, and if it is registered, the key is counted up. And conversely, the case is to delete the key if the value is 1, and count down if the value is 2 or more.

The code is as follows.

const kv = await Deno.openKv(":memory:");

// new set or countup
const res = await kv.get(["key1"]);
if(!res.versionstamp){
  await kv.set(["key1"], 0);
}else{
  await kv.set(["key1"], res.value + 1);  
}

// delete or countdown
const res = await kv.get(["key1"]);
if(res.value == 1){
  await kv.delete(["key1"]);
}else{
  await kv.set(["key1"], res.value - 1);  
}

This is because you cannot call get() from atomic(), so handling with the retrieved value is not possible.

Steps to reproduce (if applicable)

No response

Expected behavior (if applicable)

No response

Possible solution (if applicable)

We expect the following new API.

const kv = await Deno.openKv(":memory:");

// new set or countup
const res = await kv.atomic()
  .get(["key1"])
  .logic(
    (r: KvKey|KvKey[]|KvListIterator)=> {
      if(!r.versionstamp){
        return ["insert"]
      }else{
        return ["countUp", r]
      }
    },
    {
      "insert": (a: AtomicOperation) => { a.set(["key1"], 0).commit() },
      "countUp": (a: AtomicOperation, r: any) => {a.set(["key1"], r.value + 1).commit()}  
    }
  ) 
if (res.ok) {
  console.log("Set new value");
} else {
  console.log("Not set new value");
}

// delete or countdown
const res = await kv.atomic()
  .get(["key1"])
  .logic(
    (r: KvKey|KvKey[]|KvListIterator)=> {
      if(!r.versionstamp || r.value == 1){
        return ["delete"]      
      }else{
        return ["countDown", r]
      }
    },
    {
      "delete": (a: AtomicOperation) => { a.delete(["key1"]).commit() },
      "countDown": (a: AtomicOperation, r: any) => { a.set(["key1"], r.value - 1).commit() }  
    }
  ) 

if (res.ok) {
  console.log("Set new value");
} else {
  console.log("Not set new value");
}

For some purposes, there could be operations that do not call commit() at the end.

This is the concept of an API that makes check() more generalized and places more emphasis on freedom of operation.

Thanks.

Additional context

No response

igorzi commented 1 year ago

You can already do this today with atomic(). See below, which is adapted from your example. Each operation is performed atomically.

async function countUp() {
    let tx_res = { ok: false };
    while (!tx_res.ok) {
        const res = await kv.get(["key1"]);
        const atomic = kv.atomic().check(res);

        if (!res.versionstamp) {
            atomic.set(["key1"], 0);
        } else {
            atomic.set(["key1"], res.value + 1);
        }
        tx_res = await atomic.commit();
    }
}

async function countDown() {
    let tx_res = { ok: false };
    while (!tx_res.ok) {
        const res = await kv.get(["key1"]);
        const atomic = kv.atomic().check(res);

        if (res.value == 1) {
            atomic.delete(["key1"]);
        } else {
            atomic.set(["key1"], res.value - 1);
        }
        tx_res = await atomic.commit();
    }
}
igorzi commented 1 year ago

This is because you cannot call get() from atomic(), so handling with the retrieved value is not possible.

atomic() does not need to support get(). get() can be performed outside of atomic, and the result of get is passed into check(). This ensures the atomicity on the key that you're modifying.

Octo8080X commented 1 year ago

@igorzi san. Thank you, much appreciated.

We were able to verify the latest documentation. https://deno.com/manual@v1.34.1/runtime/kv/transactions

I see that the description was increased from v1.32.4. I had not noticed. I think this will solve the problem.