K2 adapter GetRowID implementation #54
Conversation
| } | ||
|
|
||
| std::future<K23SITxn> K2Adapter::beginTransaction() { | ||
| return k23si_->beginTxn(k2::K2TxnOptions{}); | ||
| } | ||
|
|
||
| k2::dto::SKVRecord K2Adapter::MakeSKVRecordWithKeysSerialized(SqlOpWriteRequest& request) { | ||
| // TODO use namespace name and table name directly? How does secondary index fit into this? | ||
| std::future<k2::GetSchemaResult> schema_f = k23si_->getSchema(request.namespace_name, request.table_name, |
There was a problem hiding this comment.
If possible, we should not get the schema information from SKV to avoid additional remote calls, but instead from the TableInfo object in the op class where the request is from (might need to twist the API a bit). The TableInfo has all the column definition. Would it be sufficient to generate the row id from the primary key values directly instead of creating a SKV record first?
| std::shared_ptr<k2::dto::Schema>& schema = schema_result.schema; | ||
| k2::dto::SKVRecord record(request.namespace_name, schema); | ||
|
|
||
| if (request.ybctid_column_value) { |
There was a problem hiding this comment.
the use case that ybctid_column_value is set has already been handled in GetRowId() method, do we need to handle this use case here as well?
There was a problem hiding this comment.
My plan is to reuse this function as part of the write op request processing. If I don't end up reusing it I can remove it from here.
| } | ||
| } else { | ||
| // Serialize key data into SKVRecord | ||
| record.serializeNext<k2::String>(request.table_name); |
There was a problem hiding this comment.
would it be possible to expose the key generation logic from the primary values directly in SKV client so that we just call that new method from k2 adapter to avoid the overhead to generate a SKV record in order to get the key?
There was a problem hiding this comment.
I agree, it would be nice to avoid the overhead of serialization to a payload here. I will look at the SKV API and see if I can find a way to do it in a clean way.
| std::future<k2::GetSchemaResult> schema_f = k23si_->getSchema(request.namespace_name, request.table_name, | ||
| request.schema_version); | ||
| // TODO ok to use it synchronously? | ||
| k2::GetSchemaResult schema_result = schema_f.get(); |
There was a problem hiding this comment.
It won't be efficient to fetch SKV schema for each call if we have to get SKV schema in this way. Image we have thousands of INSERTs, the performance would be bad. Either we use the schema information from the tableInfo directly, or you have to cache the SKV schema somewhere. Personally, the tableInfo schema is better since it is already in the op class for you to read.
There was a problem hiding this comment.
The schemas are cached on the SKVClient so I wouldn't expect too many remote network calls, but this still requires a request over the queue to seastar which will have some overhead. I'm not too worried about it right now but I will add a task to remind us to look into caching on the k2 adapter side.
* K2 adapter GetRowID implementation * Handle null values in MakeSKVRecordWithKeysSerialized. Revise some comments Co-authored-by: Justin Funston <[email protected]>
This reverts commit 05d4bfd.
* K2 adapter GetRowID implementation * Handle null values in MakeSKVRecordWithKeysSerialized. Revise some comments Co-authored-by: Justin Funston <[email protected]>
…table info in catalog manager (#56) * add logic to maintain cluster level information, namespace info, and table info in catalog manager * K2 adapter GetRowID implementation (#54) * K2 adapter GetRowID implementation * Handle null values in MakeSKVRecordWithKeysSerialized. Revise some comments Co-authored-by: Justin Funston <[email protected]> * Revert "K2 adapter GetRowID implementation (#54)" This reverts commit 05d4bfd. * address PR comments * add table id and index id as the first two partition keys for user's SKV schema * add base handler to hold common methods * check delete table data status * make SessionTransactionContext an auto object to abort transaction if it hasn't been ended in the destructor * rename finished to finished_ in SessionTransactionContext * remove delete record(s) apis from k2 adapter Co-authored-by: Justin Funston <[email protected]> Co-authored-by: Justin Funston <[email protected]>
Implementation is basically done but there are a lot of open questions I left as TODO in the code.