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

api: support IPROTO_FEATURE_SPACE_AND_INDEX_NAMES #345

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

DerekBum
Copy link

@DerekBum DerekBum commented Nov 2, 2023

Support IPROTO_FEATURE_SPACE_AND_INDEX_NAMES for Tarantool version >= 3.0.0-alpha1. It allows to use space and index names in requests instead of their IDs.

ResolveSpaceIndex function for SchemaResolver interface split into two: ResolveSpace and ResolveIndex. NamesUseSupported function added into the interface to get information if usage of space and index names is supported.

Schema structure no longer implements SchemaResolver interface.

Update Tarantool EE version 1.10.11 to 1.10.15, 2.10.0 to 2.10.8 and 2.11.0 to 2.11.1. This was done because of the one flacking test: https://github.com/tarantool/go-tarantool/actions/runs/6805504621/job/18505152412

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

Closes #338

@DerekBum DerekBum requested a review from oleg-jukovec November 2, 2023 15:25
@DerekBum
Copy link
Author

DerekBum commented Nov 2, 2023

Right now uint32 is used as a type for ids of spaces and indexes (and other iproto constants). Maybe it would be better to create out own type IprotoType uint32 and use it instead? It will help with the code readability and will also make changing types easier (in case if iproto will start accepting something other than uint32). Maybe create a type only for ids of spaces and indexes.
But it also will complicate the code in some places (as example, to decode the id, we could not use DecodeUint32. We would need to first get the result of DecodeUint32, check if err is nil, and then convert it to IprotoType).

crud/request_test.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
settings/request_test.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
@oleg-jukovec
Copy link
Collaborator

Right now uint32 is used as a type for ids of spaces and indexes (and other iproto constants). Maybe it would be better to create out own type IprotoType uint32 and use it instead?

The idea do not use a big 9-bytes values. I think we could choose a largest possible type (int64 or uint64 - we need recheck it) and use EncodeInt()/EncodeUint() to encode the values.

@DerekBum DerekBum force-pushed the DerekBum/gh-338-support-space-and-index-names branch 3 times, most recently from 71c95e0 to 3fad5b3 Compare November 7, 2023 10:05
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.

It seems to me that we need to reconsider the tests with different resolver options. Now it is not entirely clear what is being tested and why.

As example, we need to test:

  1. If NamesUseSupported() == true + space == "string" -> ResolveSpace() is not called by requests Body().
  2. If NamesUseSupported() == true + space == "string" -> ResolveIndex() is not called by requests Body().
  3. If NamesUseSupported() == false + space == "string" -> ResolveSpace() is called by requests Body() to resolve space id.
  4. If NamesUseSupported() == true + space == "string" -> ResolveIndex() is called by requests Body() to resolve index id.

It should be enough. Other tests should test what the actually test (default values, setters, invalid spaces and etc), not the behavior of resolving.

crud/request_test.go Outdated Show resolved Hide resolved
request.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
settings/request_test.go Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum/gh-338-support-space-and-index-names branch 2 times, most recently from 7cb1b5d to 42e3251 Compare November 9, 2023 08:51
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. Just a few mirror notes to fix:

CHANGELOG.md Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
settings/request_test.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum/gh-338-support-space-and-index-names branch 4 times, most recently from e025b0d to dc50888 Compare November 9, 2023 13:17
connection.go Outdated Show resolved Hide resolved
schema.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum/gh-338-support-space-and-index-names branch from dc50888 to cf194a9 Compare November 9, 2023 14:48
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Sorry, I had forgot to click "Finish your review"

CHANGELOG.md Show resolved Hide resolved
README.md Show resolved Hide resolved
request.go Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
.github/workflows/testing.yml Show resolved Hide resolved
.github/workflows/testing.yml Show resolved Hide resolved
.github/workflows/testing.yml Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum/gh-338-support-space-and-index-names branch from cf194a9 to e0e2b3e Compare November 13, 2023 09:51
Support `IPROTO_FEATURE_SPACE_AND_INDEX_NAMES` for Tarantool
version >= 3.0.0-alpha1. It allows to use space and index names in requests
instead of their IDs.

`ResolveSpaceIndex` function for `SchemaResolver` interface split into two:
`ResolveSpace` and `ResolveIndex`. `NamesUseSupported` function added into the
interface to get information if usage of space and index names is supported.

`Schema` structure no longer implements `SchemaResolver` interface.

Part of #338
Replaced `t.Errorf` + `return` by `t.Fatalf`. This made
all tests in the file follow the same code style.

Part of #338
Update Tarantool EE version 1.10.11 to 1.10.15,
2.10.0 to 2.10.8 and 2.11.0 to 2.11.1. This was done because of
the one flacking test:
https://github.com/tarantool/go-tarantool/actions/runs/6805504621/job/18505152412

Closes #338
@DerekBum DerekBum force-pushed the DerekBum/gh-338-support-space-and-index-names branch from e0e2b3e to b7dd4d8 Compare November 13, 2023 12:08
@oleg-jukovec oleg-jukovec merged commit 39ec344 into master Nov 13, 2023
22 checks passed
@oleg-jukovec oleg-jukovec deleted the DerekBum/gh-338-support-space-and-index-names branch November 13, 2023 16:36
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.

Support IPROTO_FEATURE_SPACE_AND_INDEX_NAMES
3 participants