pingcap / kvproto

Protocol buffer files for TiKV
Apache License 2.0
154 stars 220 forks source link

Add Flush and BufferGet requests for pipelined DML #1218

Closed ekexium closed 10 months ago

ekexium commented 11 months ago

Ref https://github.com/tikv/tikv/issues/16291 Don't merge it until corresponding work finishes.

you06 commented 10 months ago

Need to rebase master, I cannot build the latest TiDB with this PR because some protocols are missing.

cfzjywxk commented 10 months ago

Another way for reading a transaction's own uncommitted data in tikv:

So we could avoid introducing the BufferGet interface and do union store get within one RPC request.

ekexium commented 10 months ago

Another way for reading a transaction's own uncommitted data in tikv:

  • kv-client: For pipelined-dml read, adding a flag in the existing BatchGet request, it indicates whether the current read is membuf read in tikv or union store read in tikv.
  • tikv: reuse the same BatchGet interface.

    • storage: implement the buffer_batch_get and union_store_batch_get methods.

So we could avoid introducing the BufferGet interface and do union store get within one RPC request.

So the only difference is in the grpc level, whether to use 3 different RPCs for snapshot read, buffer read, and union read; or use one RPC for all of them and use flags to differentiate.

Seems a trade-off between interface simplicity and clarity. Imagine when we see a batch_get from logs or metrics, we would have no idea what it is reading.

cfzjywxk commented 10 months ago

We essentially need to support one RPC to complete the membuf read + snapshot read, regardless of whether or not a separate KV interface is split out. This scalability needs to be reserved. It doesn't seem to introduce too much protocol complexity to separately split out BufferBatchGetRequest, although it introduces a new interface. However, I don't think we need to introduce another UnionStoreBatchGetRequest interface.

cfzjywxk commented 10 months ago

@MyonKeminta PTAL

MyonKeminta commented 10 months ago

It doesn't seem to introduce too much protocol complexity to separately split out BufferBatchGetRequest, although it introduces a new interface. However, I don't think we need to introduce another UnionStoreBatchGetRequest interface.

I agree with this. We can regard it as an RPC interface that supports reading with the buffer, including buffer reading only and mixed (union) reading.

But my problem is whether the word "buffer" is proper here...

ekexium commented 10 months ago

@MyonKeminta any suggestions on names?