Skip to content

Conversation

@pingyu
Copy link
Contributor

@pingyu pingyu commented Oct 26, 2021

Signed-off-by: pingyu yuping@pingcap.com

Update RFC of API v2 according to latest codes.

Rendered: https://github.com/pingyu/tikv-rfcs/blob/rfc_api_v2_pd/text/0069-api-v2.md

@pingyu
Copy link
Contributor Author

pingyu commented Oct 26, 2021

/cc @zz-jason @andylokandy @iosmanthus

3. Add new keyspace.
```sh
➜ txn: cmp: create("/keyspaces/feature-store") = "0", then put /keyspaces/feature-store {"id":1}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be summarized as compare-and-swap?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this "Txn" is failed? Do we need to return an error or retry in server-side with fixed rounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to write down more detail about how to implement the compare-and-swap here, even it seems to be too verbose.

etcd doesn't provide an CAS interface actually, but a transaction interface. And I'm not very familiar to etcd. So I think more detail would help entirely confirming the correctness of this design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this "Txn" is failed? Do we need to return an error or retry in server-side with fixed rounds?

Return of Txn.Commit() will tell us success, transaction fail caused by conflict, or error occurred.
On transaction conflict, we can retry limited times (e.g. 3), return TransactionConflict to user if exceed.
On error occurred, report the error to user.

➜ txn: cmp ver("/keyspaces/feature-store") = "1" then put /keyspaces_deleted/1_feature-store {"id":1}, del /keyspaces/feature-store
```
#### Flashback
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I comprehend this as a kind of restore operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, undo a delete by mistake.

@andylokandy
Copy link
Contributor

@pingyu I've update the rfc, PTAL


>> config keyspaces delete-property <keyspace name> <property-path>
>> config keyspaces flashback <new keyspace name>
>> config keyspaces create <keyspace name> [--id <Keyspace ID>]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any restriction on the provided Keyspace ID ? For example, what if the ID is the same as another Keyspace ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if two concurrent create requests with different name but same id ?

AFAIK etcd provides transaction API similar to CAS. It seems to be difficult to solve such a race condition.

>> config keyspaces flashback <new keyspace name>
>> config keyspaces create <keyspace name> [--id <Keyspace ID>]
>> config keyspaces delete <keyspace name>
>> config keyspaces flashback [<new keyspace name>]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest describe how flashback is implemented.
Besides, flashback will delete the entry in /keyspaces_deleted (won't it ?), but PD doesn't provide such an API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, PTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to be conflicted with another concurrent create request with 1) same name, or 2) different name but same id.

Etcd's transaction API can help to deal with the same name condition. But it would be difficult to solve the same id case.

Copy link
Contributor

@andylokandy andylokandy Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we allow multiple keyspace names referring to the same id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better not to allow this.

Users would assume that two keyspaces are different if they have different names. Especially when two keyspaces have same id silently caused by the race condition. The two keyspaces will corrupt each other.

@andylokandy andylokandy mentioned this pull request Feb 23, 2022
@andylokandy andylokandy changed the title docs: Implementation detail of PD for API v2 docs: Implementation detail of API v2 Feb 23, 2022
@pingyu pingyu changed the title docs: Implementation detail of API v2 Update RFC of API v2 according to latest codes Nov 2, 2022
@pingyu pingyu requested a review from haojinming November 2, 2022 05:31
@pingyu
Copy link
Contributor Author

pingyu commented Nov 2, 2022

@iosmanthus @haojinming @zeminzhou PTAL, thanks~

Copy link
Contributor

@haojinming haojinming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ystaticy
Copy link

ystaticy commented Nov 2, 2022

LGTM

marsishandsome and others added 9 commits November 4, 2022 18:18
Signed-off-by: pingyu <yuping@pingcap.com>
* rawkv bulk load: add description for pause merge

Signed-off-by: Peng Guanwen <pg999w@outlook.com>

* Update text/0072-online-bulk-load-for-rawkv.md

Co-authored-by: Liangliang Gu <marsishandsome@gmail.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>

* Add future improvements

Signed-off-by: Peng Guanwen <pg999w@outlook.com>

Co-authored-by: Liangliang Gu <marsishandsome@gmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: Andy Lok <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: Andy Lok <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: andylokandy <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: andylokandy <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
BusyJay and others added 13 commits November 4, 2022 18:18
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: andylokandy <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: andylokandy <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: andylokandy <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: Andy Lok <andylokandy@hotmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
* RFC: In-memory Pessimistic Locks

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* clarify where to delete memory locks after writing a lock CF KV

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* Elaborate transfer leader handlings and add correctness section

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* add an addition step of proposing pessimistic locks before transferring leader

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* clarify about new leaders of region split

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* Add tracking issue link

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* update design and correctness analysis of lock migration

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* add configurations

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Co-authored-by: Xiaoguang Sun <sunxiaoguang@users.noreply.github.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
Copy link
Contributor Author

pingyu commented Nov 4, 2022

@sunxiaoguang All comments are addressed (see here. The force-push is to fix DCO error), PTAL, thanks~

Copy link
Member

@iosmanthus iosmanthus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pingyu pingyu merged commit b1a22d7 into tikv:master Nov 8, 2022
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

Successfully merging this pull request may close these issues.