driver: implement tkv driver for tarantool support #10

bigbes · 2025-10-16T11:31:48Z

New TKV driver package (driver/tkv).
Integration testing for TKV, that expects TARANTOOL_ADDR env variable
Updated .golangci.yml to fix needed packages.
Modified core driver interfaces to be compatible with tkv.
Update go module dependencies.

Also TCS is now TKV, won't be confused with 'column storage'.

Closes #TNTP-4188

Copilot

Pull Request Overview

Introduce a new Tarantool TKV driver and align core interfaces and models to support it

Add TKV driver with transactional execution and key watch support
Adjust driver interfaces (Watch signature) and data models (KeyValue, RequestResponse) for compatibility
Add integration tests against a Tarantool instance and update dependencies and lint configuration

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
driver/tkv/tkv.go	Implements TKV driver Execute and Watch, wiring to Tarantool watcher API.
driver/tkv/txn.go	Defines request/response encoding/decoding and maps Tarantool responses to tx.Response.
driver/tkv/operations.go	Encodes operations (get/put/delete) for TKV.
driver/tkv/predicate.go	Encodes predicates and maps ops/targets to TKV equivalents.
driver/tkv/integration_test.go	Integration tests validating put/get/delete, predicates, multi-op, and watch behavior.
driver/driver.go	Changes Watch API to return a channel, a stop function, and an error.
driver/etcd/etcd.go	Updates Watch signature to match the new Driver interface.
watch/event.go	Simplifies Event to contain only Prefix.
tx/requestresponse.go	Refactors RequestResponse to hold a slice of KeyValue values.
kv/kv.go	Removes CreateRevision and Version fields; keeps ModRevision.
internal/mocks/driver_mock.go	Updates mocks to match the new Watch signature.
go.mod	Adds msgpack/v5 as a direct dependency.
.golangci.yml	Allows msgpack imports and tarantool packages in tests.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

driver/tkv/operations.go

driver/tkv/tkv.go

tx/requestresponse.go

driver/driver.go

watch/event.go

- New TKV driver package (driver/tkv). - Integration testing for TKV, that expects TARANTOOL_ADDR env variable - Updated .golangci.yml to fix needed packages. - Modified core driver interfaces to be compatible with tkv. - Update go module dependencies. Also TCS is now TKV, won't be confused with 'column storage'. Closes #TNTP-4188

coveralls · 2025-10-16T12:50:00Z

Pull Request Test Coverage Report for Build 18567752915

Details

0 of 9 (0.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.6%) to 23.529%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
driver/etcd/etcd.go	0	1	0.0%
internal/mocks/driver_mock.go	0	8	0.0%

Totals
Change from base Build 18098700518:	1.6%
Covered Lines:	176
Relevant Lines:	748

💛 - Coveralls

Copilot

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

watch/event.go

driver/tkv/tkv.go

Copilot · 2025-10-16T12:53:03Z

driver/tkv/tkv.go

+	return rvChan, func() { close(isStopped) }, nil
+}


The returned stop function will panic if called more than once due to closing the same channel twice. Make it idempotent, e.g., by wrapping the close with a sync.Once or an atomic guard.

Copilot · 2025-10-16T12:53:03Z

driver/tkv/tkv.go

+// It supports optional watch configuration through the opts parameter.
+// To watch for config storage key "config.storage:" prefix should be used.
+func (d Driver) Watch(ctx context.Context, key []byte, _ ...watch.Option) (<-chan watch.Event, func(), error) {


[nitpick] Watch options are accepted but ignored (underscore parameter). If options aren't supported, either document that explicitly and validate/reject unsupported options, or plumb the options through to the watcher so callers can rely on them.

Suggested change

// It supports optional watch configuration through the opts parameter.

// To watch for config storage key "config.storage:" prefix should be used.

func (d Driver) Watch(ctx context.Context, key []byte, _ ...watch.Option) (<-chan watch.Event, func(), error) {

// The opts parameter is currently not supported; passing any options will result in an error.

// To watch for config storage key "config.storage:" prefix should be used.

func (d Driver) Watch(ctx context.Context, key []byte, opts ...watch.Option) (<-chan watch.Event, func(), error) {

if len(opts) > 0 {

return nil, nil, fmt.Errorf("watch options are not supported")

}

Copilot · 2025-10-16T12:53:03Z

tx/requestresponse.go

 import "github.com/tarantool/go-storage/kv"

 // RequestResponse represents the response for an individual transaction operation.
 type RequestResponse struct {


[nitpick] This replaces per-operation Success/Error/KeyValue with a Values slice only, removing per-request status/error reporting and changing the response shape. If this is intentional, please document the new invariants; otherwise, consider retaining per-operation status/error fields to avoid breaking consumers.

Suggested change

type RequestResponse struct {

type RequestResponse struct {

// Success indicates whether each operation succeeded.

Success []bool

// Errors contains error messages for each operation, if any.

Errors []error

Copilot · 2025-10-16T12:53:04Z

kv/kv.go

-	// CreateRevision is the revision number when this key was created.
-	CreateRevision int64
 	// ModRevision is the revision number of the last modification to this key.
 	ModRevision int64


[nitpick] Removing CreateRevision and Version is a public API break; consumers that relied on these fields will no longer compile and cannot distinguish creation vs modification semantics. Consider deprecating instead of removing or provide a migration guide (and update docs to clarify that predicates referring to 'version' now map to ModRevision).

Suggested change

ModRevision int64

ModRevision int64

// Deprecated: Use ModRevision instead. CreateRevision is the revision number when the key was created.

CreateRevision int64

// Deprecated: Use ModRevision instead. Version is the version of the key.

Version int64

oleg-jukovec

Please, rename TKV -> TcS or TCS everywhere.

oleg-jukovec

Please, update the CI impl commit title & message.

oleg-jukovec

It is a nice idea to start the CHANGELOG.md here. Or you could it fill on the first release. Up to you.

oleg-jukovec · 2025-10-20T14:39:48Z

.github/actions/prepare-ee-test-env/action.yml

+
+env:
+  # Note: Use exactly match version of tool, to avoid unexpected issues with test on CI.
+  GO_VERSION: '1.23.8'


But the library requires 1.24 in the go.mod.

oleg-jukovec · 2025-10-20T14:40:23Z

.github/actions/prepare-ee-test-env/action.yml

+    - name: Setup python
+      uses: actions/setup-python@v4
+      with:
+        python-version: '${{ env.PYTHON_VERSION }}'
+
+    - name: Setup Mage
+      run: |
+        git clone https://github.com/magefile/mage
+        cd mage
+        go run bootstrap.go
+      shell: bash
+
+    - name: Install build requirements
+      run: |
+        sudo apt -y update
+        sudo apt -y install git gcc make cmake unzip zip fish zsh
+      shell: bash


We don't need the stuff here.

oleg-jukovec · 2025-10-20T14:40:34Z

.github/actions/prepare-ee-test-env/action.yml

+        tar -xzf ${ARCHIVE_NAME}
+        rm -f ${ARCHIVE_NAME}
+        source tarantool-enterprise/env.sh
+      shell: bash


Please, add a newline.

oleg-jukovec · 2025-10-20T14:40:53Z

.github/actions/prepare-ee-test-env/action.yml

+    - name: Setup Go
+      uses: actions/setup-go@v4
+      with:
+        go-version: '${{ env.GO_VERSION }}'


Probably we don't need the Go here. It is better to install the Go in a job pipeline directly.

oleg-jukovec · 2025-10-20T14:41:19Z

.github/actions/setup-etcd/action.yml

+        ${INSTALL_PREFIX}/etcd --version
+        ${INSTALL_PREFIX}/etcdctl version 2>/dev/null || ${INSTALL_PREFIX}/etcdctl --version
+        echo "ETCD_PATH=$(echo $INSTALL_PREFIX)" >> "$GITHUB_ENV"
+        echo "${INSTALL_PREFIX}" >> "$GITHUB_PATH"


Please, add a newline.

oleg-jukovec · 2025-10-20T14:54:32Z

driver/tkv/predicate.go

+// FailedToEncodeTkvPredicateError is returned when we failed to encode tkvPredicate.
+type FailedToEncodeTkvPredicateError struct {


The same about the too long naming.

oleg-jukovec · 2025-10-20T14:56:07Z

driver/tkv/integration_test.go

+// skipIfNoTarantool skips the test if no Tarantool instance is available.
+func skipIfNoTarantool(t *testing.T) {


We need skip tests with TCS in case of Tarantool CE.

oleg-jukovec · 2025-10-20T15:02:00Z

driver/driver.go

-	Watch(ctx context.Context, key []byte, opts ...watch.Option) <-chan watch.Event
+	// The returned cleanup function should be called to stop the watch and release resources.
+	// An error is returned if the watch could not be established.
+	Watch(ctx context.Context, key []byte, opts ...watch.Option) (<-chan watch.Event, func(), error)


We need a tests that the TCS driver implements the Driver interface.

oleg-jukovec · 2025-10-20T15:03:51Z

driver/tkv/integration_test.go

@@ -0,0 +1,771 @@
+package tkv_test


There are a lack of negative tests. At least without a successful connect.

Too complex nagative scenarios should be tested with unit-tests.

oleg-jukovec · 2025-10-20T15:05:37Z

driver/tkv/tkv.go

+// New creates a new Tarantool driver instance.
+// It establishes connections to Tarantool instances using the provided addresses.
+func New(doer DoerWatcher) *Driver {


Please, add two examples of usage the driver: Execute & Watcher.

The examples could be added into a root directory of the project to example_test.go (I prefer the way).
Or in the package.

Up to you.

bigbes requested review from Copilot, elhimov and oleg-jukovec October 16, 2025 11:31

Copilot AI reviewed Oct 16, 2025

View reviewed changes

driver/tkv/operations.go Show resolved Hide resolved

driver/tkv/tkv.go Show resolved Hide resolved

tx/requestresponse.go Outdated Show resolved Hide resolved

driver/driver.go Show resolved Hide resolved

watch/event.go Outdated Show resolved Hide resolved

bigbes force-pushed the elhimov/tntp-4188-implement-tkv-driver branch from 7531a92 to c5c3cb7 Compare October 16, 2025 12:48

bigbes requested a review from Copilot October 16, 2025 12:48

Copilot AI reviewed Oct 16, 2025

View reviewed changes

bigbes added the full-ci label Oct 16, 2025

bigbes force-pushed the elhimov/tntp-4188-implement-tkv-driver branch 2 times, most recently from 870c579 to 2df1217 Compare October 16, 2025 15:43

oleg-jukovec added full-ci and removed full-ci labels Oct 16, 2025

bigbes force-pushed the elhimov/tntp-4188-implement-tkv-driver branch from 2df1217 to 2c88b0b Compare October 16, 2025 15:51

bigbes added full-ci and removed full-ci labels Oct 16, 2025

CI impl

1d5d4ed

bigbes force-pushed the elhimov/tntp-4188-implement-tkv-driver branch from 2c88b0b to 1d5d4ed Compare October 16, 2025 16:15

oleg-jukovec requested changes Oct 20, 2025

View reviewed changes

oleg-jukovec reviewed Oct 20, 2025

View reviewed changes

oleg-jukovec requested changes Oct 20, 2025

View reviewed changes

-type RequestResponse struct {
+type RequestResponse struct {
+	// Success indicates whether each operation succeeded.
+	Success []bool
+	// Errors contains error messages for each operation, if any.
+	Errors []error

		// FailedToEncodeTkvPredicateError is returned when we failed to encode tkvPredicate.
		type FailedToEncodeTkvPredicateError struct {

		// skipIfNoTarantool skips the test if no Tarantool instance is available.
		func skipIfNoTarantool(t *testing.T) {

driver: implement tkv driver for tarantool support #10

Are you sure you want to change the base?

driver: implement tkv driver for tarantool support #10

Conversation

bigbes commented Oct 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18567752915

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

oleg-jukovec left a comment

Choose a reason for hiding this comment

Uh oh!

oleg-jukovec left a comment

Choose a reason for hiding this comment

Uh oh!

oleg-jukovec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Oct 16, 2025 •

edited

Loading