Skip to content
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

CompareAndSwap will erase the expiration time of the key. #605

Open
wenlive opened this issue Oct 13, 2022 · 3 comments
Open

CompareAndSwap will erase the expiration time of the key. #605

wenlive opened this issue Oct 13, 2022 · 3 comments

Comments

@wenlive
Copy link

wenlive commented Oct 13, 2022

    In go-client V2, the function `CompareAndSwap` will erase the expiration time of the key.

E.g

rawClient.PutWithTTL key1 value1 100
//GetKeyTTL key1 ->*ttl ==100
oldValue :=rawClient.Get key1
previousValue, ok, err := rawClient.CompareAndSwap key1 oldValue newValue
//GetKeyTTL key1 ->*ttl == 0

As far as I know, tikv splices ttl in value and uses it as a compaction filter to delete the key after the life cycle.
In tikv go-client, CompareAndSwap does not specify Ttl

        reqArgs := kvrpcpb.RawCASRequest{
		Key:   key,
		Value: newValue,
	}
	if previousValue == nil {
		reqArgs.PreviousNotExist = true
	} else {
		reqArgs.PreviousValue = previousValue
	}

	req := tikvrpc.NewRequest(tikvrpc.CmdRawCompareAndSwap, &reqArgs)
	req.MaxExecutionDurationMs = uint64(client.MaxWriteExecutionTime.Milliseconds())
	resp, _, err := c.sendReq(ctx, key, req, false)

Can I add new ttl to RawCASRequest? Also what can be done to update the value using CAS and keep the historical ttl?@iosmanthus

Originally posted by @wenlive in #371 (comment)

@iosmanthus
Copy link
Member

iosmanthus commented Oct 26, 2022

RawCASRequest has a TTL parameter, this should be a bug, the new TTL should be attached to the request while the key is put with a TTL. Any bugfix pull requests are welcomed! @wenlive

@disksing
Copy link
Collaborator

If i'm understanding it correctly, the fix should be introduced in tikv-server side.

@pingyu
Copy link
Contributor

pingyu commented Oct 26, 2022

To keep the "historical ttl", I think it would better to add an argument (e.g., KeepPreviousTtl) to indicate such request, for backward compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants