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

schema: fix stale schema for metadata generation #359

Merged
merged 2 commits into from
Jun 6, 2023
Merged

schema: fix stale schema for metadata generation #359

merged 2 commits into from
Jun 6, 2023

Conversation

GRISHNOV
Copy link
Contributor

@GRISHNOV GRISHNOV commented Apr 3, 2023 •
edited
Loading

Corrects using of an stale schema to generate metadata during crud.update, crud.insert, crud.insert_*, crud.replace, crud.replace_*, crud.upsert, crud.upsert_*, crud.delete, crud.max, crud.min, crud.select,crud.pairs, crud.get working.

If the implemented fetch_latest_metadata option is used, it is guaranteed that the metadata will be up-to-date. Before receiving the space format, a mismatch check will be performed between the scheme version on all involved storage and the scheme version in the net_box connection of the router. In case of mismatch, the schema reload will be triggered.

  • Tests
  • Changelog
  • Documentation

Related with #361
Closes #236

@GRISHNOV GRISHNOV changed the title testing bugfix prototype for 236 bugfix: crud update stale schema for metadata generation Apr 3, 2023
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch from 30fc186 to b17c8be Compare April 3, 2023 13:51
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 8 times, most recently from 9d64371 to 677175d Compare April 5, 2023 16:21
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch from ba40695 to cc4ca93 Compare April 24, 2023 14:43
@GRISHNOV
Copy link
Contributor Author

I have changed the approach to fixing the bug: if the solution is suitable, I will check the rest of the crud commands to reproduce this bug. At the moment, without this fix, the bug is also reproduced on insert and insert_object

DifferentialOrange
DifferentialOrange reviewed Apr 24, 2023
View reviewed changes
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.

Thank you for finding the issue. Let's just fail on each schema mismatch and pass the reload procedure to the existing wrapper.

crud/insert.lua Outdated Show resolved Hide resolved
@DifferentialOrange
Copy link
Member

You'll also need to update this developer's doc: https://github.com/tarantool/crud/blob/master/doc/dev/schema.md

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 2 times, most recently from ad43076 to 3ee046b Compare May 10, 2023 18:47
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch from 3ee046b to 44c2a64 Compare May 18, 2023 04:54
@GRISHNOV GRISHNOV mentioned this pull request May 18, 2023
Open
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 8 times, most recently from d0b58e7 to 1193b03 Compare May 25, 2023 12:59
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 2 times, most recently from aaa36ad to f6b4455 Compare May 31, 2023 12:31
@GRISHNOV GRISHNOV marked this pull request as ready for review May 31, 2023 12:33
DifferentialOrange
DifferentialOrange reviewed May 31, 2023
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
crud/borders.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Show resolved Hide resolved
crud/insert_many.lua Show resolved Hide resolved
test/integration/simple_operations_test.lua Outdated Show resolved Hide resolved
@GRISHNOV GRISHNOV changed the title bugfix: crud DML operations stale schema for metadata generation schema: fix stale schema for metadata generation May 31, 2023
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 4 times, most recently from 77d39f6 to 1a895f8 Compare June 1, 2023 11:15
@GRISHNOV
delete: add the nil check for primary index parts
7f97ac6 
Implemented the `nil` check for primary index parts
fetching when `opts.bucket_id`  is not set.
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 3 times, most recently from b128819 to ba0f534 Compare June 5, 2023 05:57
DifferentialOrange
DifferentialOrange approved these changes Jun 5, 2023
View reviewed changes
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.

Thank you for your patch! Seems satisfying, let's improve descriptions a little bit and merge it.

CHANGELOG.md Outdated Show resolved Hide resolved
doc/dev/schema.md Outdated
@@ -92,6 +92,9 @@ Retry with reload is triggered
- ``bucket_id`` calculation has failed due to any reason;
- if updating by field name is not supported natively and id resolve
has failed;
- conditionally if the flag `fetch_latest_metadata` for DML operation that return metadata is used.
Copy link
Member

@DifferentialOrange DifferentialOrange Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This bullet list describes "Retry with reload is triggered" cases, but our one doesn't trigger operation retry. Please, move it to a new section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, moved

test/integration/simple_operations_test.lua Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/schema.lua Outdated Show resolved Hide resolved
crud/common/map_call_cases/base_postprocessor.lua Show resolved Hide resolved
crud/common/map_call_cases/batch_postprocessor.lua Outdated Show resolved Hide resolved
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch 5 times, most recently from 7a0ef63 to b072cd5 Compare June 5, 2023 15:01
@GRISHNOV
schema: fix stale schema for metadata generation
c4b037c 
Corrects using of an stale schema to generate metadata for
`crud.update`, `crud.insert`, `crud.insert_*`, `crud.replace`,
`crud.replace_*`, `crud.upsert`, `crud.upsert_*`, `crud.delete`,
`crud.max`, `crud.min`, `crud.select`, `crud.pairs` and `crud.get`.

If the implemented `fetch_latest_metadata` option is used,
it is guaranteed that the metadata will be up-to-date.
Before receiving the space format, a mismatch check will be
performed between the scheme version on all involved storage
and the scheme version in the net_box connection of the router.
In case of mismatch, the schema reload will be triggered.

Closes #236
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-236-update-stale-schema-metadata-gen branch from b072cd5 to c4b037c Compare June 5, 2023 18:25
@DifferentialOrange DifferentialOrange merged commit 2675925 into master Jun 6, 2023
@DifferentialOrange DifferentialOrange deleted the igrishnov/gh-236-update-stale-schema-metadata-gen branch June 6, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DifferentialOrange DifferentialOrange DifferentialOrange approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crud.update() may use stale schema for metadata generation
2 participants
@GRISHNOV @DifferentialOrange