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.

Sign up for GitHub

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

Jump to bottom

Remove select with custom merger #365

Open
DifferentialOrange opened this issue Jun 13, 2023 · 1 comment · May be fixed by #391
Open

Remove select with custom merger #365

DifferentialOrange opened this issue Jun 13, 2023 · 1 comment · May be fixed by #391
Assignees
@DifferentialOrange
Labels
code health Improve code readability, simplify maintenance and so on question Further information is requested

Comments

@DifferentialOrange
Copy link
Member

crud uses tuple-merger for select. Built-in merger is provided for the following versions

crud/crud/common/utils.lua

Lines 589 to 599 in 2d3d479

enabled_tarantool_features.builtin_merger = is_version_ge(major, minor, patch, suffix,
2, 6, 0, nil)
or is_version_in_range(major, minor, patch, suffix,
2, 5, 1, nil,
2, 5, math.huge, nil)
or is_version_in_range(major, minor, patch, suffix,
2, 4, 2, nil,
2, 4, math.huge, nil)
or is_version_in_range(major, minor, patch, suffix,
2, 3, 3, nil,
2, 3, math.huge, nil)

and external merger can be provided for the following versions

crud/crud/common/utils.lua

Lines 607 to 620 in 2d3d479

enabled_tarantool_features.external_merger = is_version_ge(major, minor, patch, suffix,
2, 7, 0, nil)
or is_version_in_range(major, minor, patch, suffix,
2, 6, 1, nil,
2, 6, math.huge, nil)
or is_version_in_range(major, minor, patch, suffix,
2, 5, 2, nil,
2, 5, math.huge, nil)
or is_version_in_range(major, minor, patch, suffix,
2, 4, 3, nil,
2, 4, math.huge, nil)
or is_version_in_range(major, minor, patch, suffix,
1, 10, 8, nil,
1, 10, math.huge, nil)

There is a separate select implementation without merger for older versions without internal merger support (<= 1.10.7, == 2.5.0, >= 2.4.0, <= 2.4.1, >= 2, <= 2.3.2) and without internal merger if external not provided (< 2.0.0): select_old.

All versions that require select_old are out of support for a long time: it covers <= 1.10.7 and >=2, <= 2.5.2 releases. There is also the >= 1.10.8, <= 1.10.15version range withselect_old` support.

Supporting two select implementation is really bothersome. A lot of time spent to update a code which doesn't seems relevant anymore. I propose to remove this code.

The course of action is as follows.

  1. Ensure that it is fine to drop support of Tarantool releases <= 1.10.7 >=2, <= 2.5.2.
  2. Ensure that it is fine to require all users of Tarantool releases >= 1.10.8, <= 1.10.15to installtuple-merger` as well. (We already seem to do that in CMake:

    crud/CMakeLists.txt

    Lines 105 to 107 in 2d3d479

    execute_process(
    COMMAND bash "-c" "tarantoolctl rocks install tuple-merger 0.0.2"
    )
    ).
  3. Migrate tuple-merger dependency from CMake to rockspec, as well as tuple-keydef one.
  4. Remove select_old code and compatibility layers in code. Remove related tests and CI presets. Rework "if there is a merger" condition to strict assertion.
  5. Tag a new module major version.
  6. File a ticket related to removing tuple-merger dependency in the future: after Tarantool releases >= 1.10.8, <= 1.10.15` support drop there won't be any need for external merger in supported versions.
@DifferentialOrange DifferentialOrange added question Further information is requested teamE code health Improve code readability, simplify maintenance and so on labels Jun 13, 2023
@DifferentialOrange DifferentialOrange self-assigned this Oct 18, 2023
DifferentialOrange added a commit that referenced this issue Oct 18, 2023
@DifferentialOrange
ci: drop testing on older versions
31d81c7 
Versions in question are not supported for at least two years from
now [1].

1. https://www.tarantool.io/en/doc/latest/release/calendar/

Part of #365
DifferentialOrange added a commit that referenced this issue Oct 18, 2023
@DifferentialOrange
rock: unlock tuple-keydef and tuple-merger version deps
abad36e 
Since Tarantool 1.10.7 and older and Tarantools between 2.0 and 2.5.2
does not have required symbols, we drop their support in the commit too.

1. https://www.tarantool.io/en/doc/latest/release/calendar/

Part of #365
DifferentialOrange added a commit that referenced this issue Oct 18, 2023
@DifferentialOrange
crud: require tuple-keydef and tuple-merger
a11e932 
This patch removes compatibility code that emulated `tuple-keydef` and
`tuple-merger` support when they were missing. Now it is required
to have this modules either internally r externally.

Closes #365
@DifferentialOrange DifferentialOrange linked a pull request Oct 18, 2023 that will close this issue
Draft
DifferentialOrange added a commit that referenced this issue Oct 18, 2023
@DifferentialOrange
ci: drop testing on older versions
96d9c29 
Versions in question are not supported for at least two years from
now [1].

1. https://www.tarantool.io/en/doc/latest/release/calendar/

Part of #365
DifferentialOrange added a commit that referenced this issue Oct 18, 2023
@DifferentialOrange
rock: unlock tuple-keydef and tuple-merger version deps
113d887 
Since Tarantool 1.10.7 and older and Tarantools between 2.0 and 2.5.2
does not have required symbols, we drop their support in the commit too.

1. https://www.tarantool.io/en/doc/latest/release/calendar/

Part of #365
DifferentialOrange added a commit that referenced this issue Oct 18, 2023
@DifferentialOrange
crud: require tuple-keydef and tuple-merger
9e275e4 
This patch removes compatibility code that emulated `tuple-keydef` and
`tuple-merger` support when they were missing. Now it is required
to have this modules either internally r externally.

Closes #365
@Totktonada
Copy link
Member

Totktonada commented Oct 19, 2023 •
edited
Loading

1.10 series at whole is going the end of support, so I would re-consider tuple-keydef and tuple-merger from modules for old tarantool versions to modules to update a component without updating tarantool. So, I would propose an alternative plan.

  1. Use require('key_def') and require('merger') in crud.
  2. Drop tuple-keydef and tuple-merger dependencies.
  3. Ship override/key_def.so and override/merger.so from the tuple.* modules (see Builtin modules overloading tarantool#7774).

@Totktonada Totktonada mentioned this issue Oct 19, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DifferentialOrange DifferentialOrange

Labels
code health Improve code readability, simplify maintenance and so on question Further information is requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Drop old select tarantool/crud
3 participants
@Totktonada @DifferentialOrange @TarantoolBot