Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

box: first box and box.info implementation #414

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

KaymeKaydex
Copy link

@KaymeKaydex KaymeKaydex commented Oct 19, 2024

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

Closes: #410

@KaymeKaydex KaymeKaydex force-pushed the feature/box branch 10 times, most recently from 5d9ad18 to e6327cd Compare October 19, 2024 13:55
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

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:

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

box/info.go Outdated Show resolved Hide resolved
@KaymeKaydex
Copy link
Author

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:

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
Copy link
Collaborator

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:

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
Copy link
Author

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:

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
Copy link
Collaborator

oleg-jukovec commented Oct 20, 2024

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
Copy link
Author

@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
Copy link
Author

@DerekBum , please also look at pr

Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you for the patch!

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

box/box.go Outdated Show resolved Hide resolved
box/example_test.go Show resolved Hide resolved
box/info.go Outdated Show resolved Hide resolved
box/common.go Outdated Show resolved Hide resolved
box/tarantool_test.go Outdated Show resolved Hide resolved
box/info.go Outdated Show resolved Hide resolved
box/info.go Show resolved Hide resolved
box/info.go Show resolved Hide resolved
box/info.go Show resolved Hide resolved
box/common.go Outdated Show resolved Hide resolved
@oleg-jukovec
Copy link
Collaborator

You could rebase on the master to make tests green.

box/box_test.go Outdated Show resolved Hide resolved
box/example_test.go Show resolved Hide resolved
box/info.go Show resolved Hide resolved
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you!

Please, squash commits into a single/few with nice formatted title & message (see examples in the repo) since we use "Rebase and merge" here.

@oleg-jukovec oleg-jukovec requested a review from DerekBum December 9, 2024 11:20
box/box.go Outdated Show resolved Hide resolved
@KaymeKaydex
Copy link
Author

You could rebase on the master to make tests green.

done

@KaymeKaydex KaymeKaydex force-pushed the feature/box branch 2 times, most recently from 53de86a to f94f942 Compare December 10, 2024 09:19
@oleg-jukovec
Copy link
Collaborator

Please, update the commit message:

box: first box and box.info implementation

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

And I'll merge it.

@KaymeKaydex KaymeKaydex changed the title feature/box: first box and box.info implementation box: first box and box.info implementation Dec 10, 2024
@KaymeKaydex
Copy link
Author

Please, update the commit message:


box: first box and box.info implementation



Implemented the box interface for tarantool with a small number

of fields, which in the future can be supplemented.

And I'll merge it.

Done

@oleg-jukovec
Copy link
Collaborator

Please, update the commit message:


box: first box and box.info implementation



Implemented the box interface for tarantool with a small number

of fields, which in the future can be supplemented.

And I'll merge it.

Done

Still actual )

Implemented the box interface for tarantool with a small number of fields, which in the future can be supplemented.
@KaymeKaydex
Copy link
Author

Please, update the commit message:


box: first box and box.info implementation



Implemented the box interface for tarantool with a small number

of fields, which in the future can be supplemented.

And I'll merge it.

Done

Still actual )

fixed it, ide settings was broken

@oleg-jukovec
Copy link
Collaborator

Thank you!

@oleg-jukovec oleg-jukovec merged commit ebdf986 into tarantool:master Dec 10, 2024
20 checks passed
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.

api: add box.info method
3 participants