feature/box: first box and box.info implementation #414

KaymeKaydex · 2024-10-19T10:09:58Z

I implemented the box interface for tarantool with a small number of fields, which in the future can be supplemented

I didn't forget about (remove if it is not applicable):

Tests (see documentation for a testing package)
Changelog (see documentation for changelog format)
Documentation (see documentation for documentation style guide)

Related issues: #410

implemented the box interface for tarantool with a small number of fields, which in the future can be supplemented

oleg-jukovec

The problem with this approach is that it cannot be extended. As example:

How to use a context with the method? How to cancel the request?
How to make an async request?

It's better to implement a custom request type, as it is already done in the crud and the arrow subpackages. It is possible to implement a custom response type for a request too:

go-tarantool/example_test.go

Line 577 in 8cf8673

exResp, ok := resp.(*tarantool.ExecuteResponse)

oleg-jukovec · 2024-10-19T17:38:12Z

box/info.go

+	// Cluster information, including cluster UUID.
+	Cluster ClusterInfo `msgpack:"cluster"`


In the Tarantool 3 the field has name replicaset.

there should be a custom decoder for this type or extend the type to avoid problems, this implementation is rather minimal

Well, that is, I propose in this case, for example, to remove this field altogether, leaving a minimal non-conflicting implementation

KaymeKaydex · 2024-10-20T09:07:05Z

The problem with this approach is that it cannot be extended. As example:
1. How to use a context with the method? How to cancel the request?

2. How to make an async request?
It's better to implement a custom request type, as it is already done in the crud and the arrow subpackages. It is possible to implement a custom response type for a request too:

go-tarantool/example_test.go

Line 577 in 8cf8673

exResp, ok := resp.(*tarantool.ExecuteResponse)

I absolutely agree about the context and even think that it should be mandatory when calling any method, but I see the asynchronous implementation of using box as redundant, the asynchronous implementation may be cheaper than a goroutine, but still

I mean the complexity of using asynchronous calls to the future

oleg-jukovec · 2024-10-20T10:59:13Z

The problem with this approach is that it cannot be extended. As example:
1. How to use a context with the method? How to cancel the request?

2. How to make an async request?
It's better to implement a custom request type, as it is already done in the crud and the arrow subpackages. It is possible to implement a custom response type for a request too:

go-tarantool/example_test.go

Line 577 in 8cf8673

exResp, ok := resp.(*tarantool.ExecuteResponse)
I absolutely agree about the context and even think that it should be mandatory when calling any method, but I see the asynchronous implementation of using box as redundant, the asynchronous implementation may be cheaper than a goroutine, but still

I mean the complexity of using asynchronous calls to the future

Maybe the user will use it this way, and maybe he will want it differently. And considering that we are developing a public library, I am not so sure about the answer.

In any case, I would prefer to do it a way that is already implemented in the crud or arrow subpackage: create a custom request and response. In our experience, this is easier to improve and maintain. It is more universal in practice, it is easier to implement custom connections/types/pools with custom API that used requests, it's easier to test etc.

KaymeKaydex · 2024-10-20T12:22:34Z

The problem with this approach is that it cannot be extended. As example:
1. How to use a context with the method? How to cancel the request?

2. How to make an async request?
It's better to implement a custom request type, as it is already done in the crud and the arrow subpackages. It is possible to implement a custom response type for a request too:

go-tarantool/example_test.go

Line 577 in 8cf8673

exResp, ok := resp.(*tarantool.ExecuteResponse)
I absolutely agree about the context and even think that it should be mandatory when calling any method, but I see the asynchronous implementation of using box as redundant, the asynchronous implementation may be cheaper than a goroutine, but still
I mean the complexity of using asynchronous calls to the future
Maybe the user will use it this way, and maybe he will want it differently. And considering that we are developing a public library, I am not so sure about the answer.

In any case, I would prefer to do it a way that is already implemented in the crud or arrow subpackage: create a custom request and response. In our experience, this is easier to improve and maintain. It is more universal in practice, it is easier to implement custom connections/types/pools with custom API that used requests, it's easier to test etc.

yes, I understand, but the ease of use of the box is lost in this case, I understood the fixes and am implementing them. I just wanted to note that, just like an active user of the library, it would be easier for me to add a context to the Info() signature, if necessary, launch a goroutine

oleg-jukovec · 2024-10-20T15:09:20Z

I agree that it is easier at the moment, but not in the long term. As example, think about adding context here to all these methods with backward compatibility.

go-tarantool/connector.go

Lines 20 to 128 in 8cf8673

    
           // Deprecated: the method will be removed in the next major version, 
        
           // use a PingRequest object + Do() instead. 
        
           Ping() ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a SelectRequest object + Do() instead. 
        
           Select(space, index interface{}, offset, limit uint32, iterator Iter, 
        
           	key interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an InsertRequest object + Do() instead. 
        
           Insert(space interface{}, tuple interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a ReplicaRequest object + Do() instead. 
        
           Replace(space interface{}, tuple interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a DeleteRequest object + Do() instead. 
        
           Delete(space, index interface{}, key interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a UpdateRequest object + Do() instead. 
        
           Update(space, index interface{}, key interface{}, ops *Operations) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a UpsertRequest object + Do() instead. 
        
           Upsert(space interface{}, tuple interface{}, ops *Operations) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a CallRequest object + Do() instead. 
        
           Call(functionName string, args interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a Call16Request object + Do() instead. 
        
           Call16(functionName string, args interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a Call17Request object + Do() instead. 
        
           Call17(functionName string, args interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an EvalRequest object + Do() instead. 
        
           Eval(expr string, args interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an ExecuteRequest object + Do() instead. 
        
           Execute(expr string, args interface{}) ([]interface{}, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a SelectRequest object + Do() instead. 
        
           GetTyped(space, index interface{}, key interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a SelectRequest object + Do() instead. 
        
           SelectTyped(space, index interface{}, offset, limit uint32, iterator Iter, key interface{}, 
        
           	result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an InsertRequest object + Do() instead. 
        
           InsertTyped(space interface{}, tuple interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a ReplaceRequest object + Do() instead. 
        
           ReplaceTyped(space interface{}, tuple interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a DeleteRequest object + Do() instead. 
        
           DeleteTyped(space, index interface{}, key interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a UpdateRequest object + Do() instead. 
        
           UpdateTyped(space, index interface{}, key interface{}, ops *Operations, 
        
           	result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a CallRequest object + Do() instead. 
        
           CallTyped(functionName string, args interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a Call16Request object + Do() instead. 
        
           Call16Typed(functionName string, args interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a Call17Request object + Do() instead. 
        
           Call17Typed(functionName string, args interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an EvalRequest object + Do() instead. 
        
           EvalTyped(expr string, args interface{}, result interface{}) error 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an ExecuteRequest object + Do() instead. 
        
           ExecuteTyped(expr string, args interface{}, 
        
           	result interface{}) (SQLInfo, []ColumnMetaData, error) 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a SelectRequest object + Do() instead. 
        
           SelectAsync(space, index interface{}, offset, limit uint32, iterator Iter, 
        
           	key interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an InsertRequest object + Do() instead. 
        
           InsertAsync(space interface{}, tuple interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a ReplaceRequest object + Do() instead. 
        
           ReplaceAsync(space interface{}, tuple interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a DeleteRequest object + Do() instead. 
        
           DeleteAsync(space, index interface{}, key interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a UpdateRequest object + Do() instead. 
        
           UpdateAsync(space, index interface{}, key interface{}, ops *Operations) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a UpsertRequest object + Do() instead. 
        
           UpsertAsync(space interface{}, tuple interface{}, ops *Operations) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a CallRequest object + Do() instead. 
        
           CallAsync(functionName string, args interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a Call16Request object + Do() instead. 
        
           Call16Async(functionName string, args interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use a Call17Request object + Do() instead. 
        
           Call17Async(functionName string, args interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an EvalRequest object + Do() instead. 
        
           EvalAsync(expr string, args interface{}) *Future 
        
           // Deprecated: the method will be removed in the next major version, 
        
           // use an ExecuteRequest object + Do() instead. 
        
           ExecuteAsync(expr string, args interface{}) *Future

+ Connection + ConnectionPool.

API with requests vs API with ready to call methods a much easier to maintain and to extend over time.

KaymeKaydex · 2024-11-29T13:22:21Z

@oleg-jukovec i added request and response types with tarantool.Request face, so sugar logic just use them and wrap to more easy logical struct

KaymeKaydex · 2024-12-04T10:08:42Z

@DerekBum , please also look at pr

oleg-jukovec

Thank you for the patch!

I agree with the idea, let's just resolve the comments.

oleg-jukovec · 2024-12-04T15:36:35Z

box/box.go

+// Box defines an interface for interacting with a Tarantool instance.
+// It includes the Info method, which retrieves instance information.
+type Box interface {
+	Info() (Info, error) // Retrieves detailed information about the Tarantool instance.
+}
+
+// box is a concrete implementation of the Box interface.
+// It holds a connection to the Tarantool instance via the Doer interface.
+type box struct {
+	conn tarantool.Doer // Connection interface for interacting with Tarantool.
+}
+
+// By returns a new instance of the box structure, which implements the Box interface.
+func By(conn tarantool.Doer) Box {
+	return &box{
+		conn: conn, // Assigns the provided Tarantool connection.
+	}
+}


I think it's better to work here without an interface with just a concrete implementation.

Suggested change

// Box defines an interface for interacting with a Tarantool instance.

// It includes the Info method, which retrieves instance information.

type Box interface {

Info() (Info, error) // Retrieves detailed information about the Tarantool instance.

}

// box is a concrete implementation of the Box interface.

// It holds a connection to the Tarantool instance via the Doer interface.

type box struct {

conn tarantool.Doer // Connection interface for interacting with Tarantool.

}

// By returns a new instance of the box structure, which implements the Box interface.

func By(conn tarantool.Doer) Box {

return &box{

conn: conn, // Assigns the provided Tarantool connection.

}

}

// Box is a helper that wraps for box.* requests.

// It holds a connection to the Tarantool instance via the Doer interface.

type Box struct {

conn tarantool.Doer // Connection interface for interacting with Tarantool.

}

// By returns a new instance of the box structure, which implements the Box interface.

func New(conn tarantool.Doer) Box {

return &Box{

conn: conn, // Assigns the provided Tarantool connection.

}

}

oleg-jukovec · 2024-12-04T15:37:35Z

box/example_test.go

+
+func Example() {


Please, add a separate example for the Box.Info() too.

oleg-jukovec · 2024-12-04T15:38:15Z

box/info.go

+// Info retrieves the current information of the Tarantool instance.
+// It calls the "box.info" function and parses the result into the Info structure.
+func (b *box) Info() (Info, error) {
+	var infoResp InfoResponse
+
+	// Call "box.info" to get instance information from Tarantool.
+	fut := b.conn.Do(NewInfoRequest())
+
+	// Parse the result into the Info structure.
+	err := fut.GetTyped(&infoResp)
+	if err != nil {
+		return Info{}, err
+	}
+
+	// Return the parsed info and any potential error.
+	return infoResp.Info, err
+}
+


Please, move it to box.go. The request/response is ok here.

oleg-jukovec · 2024-12-04T15:38:46Z

box/common.go

-> request.go

oleg-jukovec · 2024-12-04T15:39:56Z

box/tarantool_test.go

+	// check all fields run correctly
+	_, err = uuid.Parse(info.UUID)
+	require.NoErrorf(t, err, "validate instance uuid is valid")
+
+	require.NotEmpty(t, info.Version)
+	// check that pid parsed correctly


Suggested change

// check all fields run correctly

_, err = uuid.Parse(info.UUID)

require.NoErrorf(t, err, "validate instance uuid is valid")

require.NotEmpty(t, info.Version)

// check that pid parsed correctly

// Check all fields run correctly.

_, err = uuid.Parse(info.UUID)

require.NoErrorf(t, err, "validate instance uuid is valid")

require.NotEmpty(t, info.Version)

// Check that pid parsed correctly.

oleg-jukovec · 2024-12-04T15:40:30Z

box/info.go

+// ClusterInfo represents information about the cluster.
+// It contains the unique identifier (UUID) of the cluster.
+type ClusterInfo struct {
+	UUID string `msgpack:"uuid"`
+}


Is it used anywhere?

oleg-jukovec · 2024-12-04T15:41:44Z

box/info.go

+	// The node ID (nullable).
+	ID *int `msgpack:"id"`


If I'm not mistaken replicaset id starts from 1, so 0 is a valid-nil value. If I'm wrong, there's no need to fix it.

Suggested change

// The node ID (nullable).

ID *int `msgpack:"id"`

// The node ID (nullable).

ID int `msgpack:"id"`

oleg-jukovec · 2024-12-04T15:44:41Z

box/info.go

+	LSN uint64 `msgpack:"lsn"`
+}


With a custom decoder you could be able to decode replicaset.uuid and cluster.uuid at a same field. But I don't mind if it isn't implemented here.

oleg-jukovec · 2024-12-04T15:46:15Z

box/info.go

+	if err != nil {
+		return err
+	}
+
+	if arrayLen != 1 {
+		return fmt.Errorf("protocol violation; expected 1 array entry, got %d", arrayLen)
+	}


You could add tests for this cases.

oleg-jukovec · 2024-12-04T15:46:42Z

box/common.go

+// Type returns IPROTO type for request.
+func (req baseRequest) Type() iproto.Type {
+	return req.impl.Type()
+}
+
+// Ctx returns a context of request.
+func (req baseRequest) Ctx() context.Context {
+	return req.impl.Ctx()
+}
+
+// Async returns request expects a response.
+func (req baseRequest) Async() bool {
+	return req.impl.Async()
+}
+
+// Response creates a response for the baseRequest.
+func (req baseRequest) Response(header tarantool.Header,
+	body io.Reader) (tarantool.Response, error) {
+	return req.impl.Response(header, body)
+}


And a test for that too.

KaymeKaydex force-pushed the feature/box branch 9 times, most recently from c643841 to 5d9ad18 Compare October 19, 2024 13:55

feature/box: first box and box.info implementation

e6327cd

implemented the box interface for tarantool with a small number of fields, which in the future can be supplemented

KaymeKaydex force-pushed the feature/box branch from 5d9ad18 to e6327cd Compare October 19, 2024 13:55

oleg-jukovec requested changes Oct 19, 2024

View reviewed changes

maksim.konovalov added 2 commits November 29, 2024 00:15

Merge branch 'master' into feature/box

e8c40da

Add backward request and response logic

b6d6ec9

KaymeKaydex requested a review from oleg-jukovec November 29, 2024 09:40

maksim.konovalov added 4 commits November 29, 2024 12:48

Fix imports sort

ed9f690

Check err on GetTyped in example

8a01943

Fix lint comment

e80e9d6

Fix lint comment

399ea97

oleg-jukovec reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/box: first box and box.info implementation #414

feature/box: first box and box.info implementation #414

KaymeKaydex commented Oct 19, 2024 •

edited

Loading

oleg-jukovec left a comment

oleg-jukovec Oct 19, 2024

KaymeKaydex Oct 20, 2024

KaymeKaydex Oct 20, 2024

KaymeKaydex commented Oct 20, 2024

oleg-jukovec commented Oct 20, 2024

KaymeKaydex commented Oct 20, 2024

oleg-jukovec commented Oct 20, 2024 •

edited

Loading

KaymeKaydex commented Nov 29, 2024

KaymeKaydex commented Dec 4, 2024

oleg-jukovec left a comment

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

oleg-jukovec Dec 4, 2024

		// Cluster information, including cluster UUID.
		Cluster ClusterInfo `msgpack:"cluster"`

feature/box: first box and box.info implementation #414

Are you sure you want to change the base?

feature/box: first box and box.info implementation #414

Conversation

KaymeKaydex commented Oct 19, 2024 • edited Loading

oleg-jukovec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaymeKaydex commented Oct 20, 2024

oleg-jukovec commented Oct 20, 2024

KaymeKaydex commented Oct 20, 2024

oleg-jukovec commented Oct 20, 2024 • edited Loading

KaymeKaydex commented Nov 29, 2024

KaymeKaydex commented Dec 4, 2024

oleg-jukovec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaymeKaydex commented Oct 19, 2024 •

edited

Loading

oleg-jukovec commented Oct 20, 2024 •

edited

Loading