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

Support custom sharding func #239

Merged
merged 6 commits into from
Feb 21, 2022
Merged

Support custom sharding func #239

merged 6 commits into from
Feb 21, 2022
+1,859 −522

Conversation

AnaNek
Copy link
Contributor

@AnaNek AnaNek commented Nov 24, 2021 •
edited
Loading

CRUD uses strcrc32 as sharding function by default.
The reason why using of strcrc32 is undesirable is that
this sharding function is not consistent for cdata numbers.
In particular, it returns 3 different values for normal Lua
numbers like 123, for unsigned long long cdata
(like 123ULL, or ffi.cast('unsigned long long',
123)), and for signed long long cdata (like 123LL, or
ffi.cast('long long', 123)).

We should leave strcrc32 function as sharding function
default to support existing applications using old version of
CRUD.

There is _ddl_sharding_func space as part of ddl API.
This space contains sharding functions specified by user.
So user can specify more reliable sharding function
(like mpcrc32) using ddl API and crud will know about
that and will calculate bucket_id by specified sharding func.

Closes #237

@AnaNek AnaNek requested review from Totktonada and ligurio November 24, 2021 04:38
ligurio
ligurio suggested changes Nov 24, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
crud/common/sharding/sharding_func.lua Outdated Show resolved Hide resolved
crud/common/sharding/sharding_func.lua Outdated Show resolved Hide resolved
crud/common/sharding/sharding_func.lua Show resolved Hide resolved
ligurio
ligurio suggested changes Nov 24, 2021
View reviewed changes
crud/common/sharding/sharding_func.lua Outdated Show resolved Hide resolved
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch from 9e82913 to c8674cf Compare December 1, 2021 02:10
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch from c8674cf to 3b3e08a Compare December 8, 2021 07:26
ligurio
ligurio suggested changes Dec 13, 2021
View reviewed changes
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

Thanks for changes!
This patchset contains:

  • you own changes with implementation of sharding function support
  • copy-pasted code from DDL
  • my own code for implementation of secondary sharding keys support (because you are moving module sharding_key.lua to another directory)

I don't want to spend time on review my own code and code copy-pasted from DDL. Could you split all patchset for at least three commits:

  • moving sharding_key.lua to a separate dir, commit marked as a part of issue Support custom sharding func #239. Git will show file as a renamed and nothing to review here.
  • commit with copy-pasted functions from DDL (like is_called and other with their tests)
  • your own changes with sharding function support

if you will do it you will simplify review a lot

README.md Outdated Show resolved Hide resolved
crud/common/sharding/sharding.lua Outdated Show resolved Hide resolved
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch 3 times, most recently from 2ead6bc to 78aceb3 Compare December 22, 2021 13:31
@ligurio ligurio self-requested a review December 27, 2021 08:38
@ligurio
Copy link
Member

ligurio commented Dec 27, 2021

  • Commit messages must follow to our guideline.
  • Commit "Replace common sharding methods in new folder sharding" renames "crud/common/sharding.lua" to "crud/common/sharding/sharding.lua". What is a point to do it? Could we choose a better name for module?

ligurio
ligurio suggested changes Dec 27, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
crud/common/sharding/sharding.lua Outdated Show resolved Hide resolved
@ligurio
Copy link
Member

ligurio commented Dec 27, 2021

I don't want to spend time on review my own code and code copy-pasted from DDL. Could you split all patchset for at least three commits:

  • moving sharding_key.lua to a separate dir, commit marked as a part of issue Support custom sharding func #239. Git will show file as a renamed and nothing to review here.
  • commit with copy-pasted functions from DDL (like is_called and other with their tests)
    your own changes with sharding function support

It is still valid.

@AnaNek AnaNek force-pushed the support-custom-sharding-func branch from 78aceb3 to b13e87b Compare December 29, 2021 08:45
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch 11 times, most recently from 68b5b5f to 1e7bf0b Compare January 12, 2022 10:08
@ligurio ligurio mentioned this pull request Jan 14, 2022
Closed
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch 5 times, most recently from 7c1a1ba to 8c9f4a0 Compare January 31, 2022 14:19
ligurio
ligurio suggested changes Jan 31, 2022
View reviewed changes
crud.lua Show resolved Hide resolved
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch 3 times, most recently from 4704a94 to 2161b99 Compare February 1, 2022 12:05
@AnaNek AnaNek requested a review from Totktonada February 1, 2022 12:19
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch 2 times, most recently from 45f9a7d to 21fe7bf Compare February 8, 2022 11:17
@AnaNek AnaNek requested a review from ligurio February 10, 2022 10:35
Totktonada
Totktonada reviewed Feb 16, 2022
View reviewed changes
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

While I'm looking on the changes, several minor comments about the 'Separate common sharding metadata methods from sharding key methods' commit.

crud/common/sharding/sharding_metadata.lua Show resolved Hide resolved
crud/common/sharding/sharding_metadata.lua Outdated Show resolved Hide resolved
crud/common/sharding_key.lua Outdated Show resolved Hide resolved
Totktonada
Totktonada reviewed Feb 16, 2022
View reviewed changes
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

One comment on 'Add support of custom sharding func for crud methods'.

README.md Outdated Show resolved Hide resolved
Totktonada
Totktonada approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

LGTM.

Please, fix several remaining commits. After this it is okay to push from me. Please, review with @ligurio before that.

IMHO, the preliminary commits are good enough now: I'm able to spot what is going on at least. I propose to don't pay more time on polishing them in terms on code layout and so on.

The logic of fetching / caching looks okay too. We'll meet more problems, when will implement automatic reloading, but now it does not require more quirks.

@AnaNek
Replace sharding methods in new directory sharding
e820c9f 
Since this patch presents the implementation of support of
DDL sharding function, the number of files associated with
sharding will increase in the `common` folder. Therefore,
a separate directory was created for files containing
methods for working with sharding.

Part of #237
@AnaNek
Copy the content of sharding_key.lua file to `sharding_metadata.lua…
cc69113 
…` file

PR #181 introduced support of DDL sharding keys. Implementation of
sharding keys support contains methods that are common to support
sharding keys and sharding functions. That's why a separate file
`sharding_metadata.lua` was created to contain common methods.
This commit relocates functions for fetching sharding key from
`sharding_key.lua` file to `sharding_metadata.lua` file
to simplify a reviewer's life and display the history of
changes relative to PR #181 in the following commits.

Part of #237
@AnaNek
Separate common sharding metadata methods from sharding key methods
02e6fb1 
It would be more efficient to get sharding keys and
sharding functions from storage in one `fetch_on_storage`
function call. So, this function is common for sharding
keys and sharding functions support. As well as functions
for fetching on router. These methods are introduced
in `sharding_metadata` module. Methods for working
with sharding key structure are introduced in
sharding key module.

Part of #237
@AnaNek
Fix filling sharding key cache when sharding key data is incorrect
2ed4841 
Sharding key cache contains sharding key structures
separated by space name. Before this fix if sharding key
was incorrect for some space an error was returned and
sharding keys that were after the incorrect one
did not get into the cache. The solution is to
create a variable for an error and write a
message to it if an error occurred for the space
we are working with, output a warning for other spaces.

Part of #237
@AnaNek
Add functions from ddl for sharding func module
6b7151c 
In DDL PR tarantool/ddl#72
methods for checking and extracting sharding function
were introduced. These methods are needed for supporting
sharding functions in CRUD as well. In this commit these
methods were coppied from DDL and covered by unit tests.

Part of #237
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch from 21fe7bf to 37e37c0 Compare February 21, 2022 13:22
@AnaNek
Add support of custom sharding func for crud methods
2887e18 
This commit introduces modifications in functions
for fetching sharding metadata on storage and router
to get sharding function. Function `sharding.key_get_bucket_id`
calculates bucket_id using DDL sharding function if
sharding function exist for specified space. Description in
documentation, integration and unit tests are added
as well.

Closes #237
@AnaNek AnaNek force-pushed the support-custom-sharding-func branch from 37e37c0 to 2887e18 Compare February 21, 2022 13:34
ligurio
ligurio approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

LGTM

@Totktonada Totktonada merged commit 263900b into master Feb 21, 2022
@Totktonada Totktonada deleted the support-custom-sharding-func branch February 21, 2022 17:20
DifferentialOrange added a commit that referenced this pull request Apr 8, 2022
@DifferentialOrange
changelog: fix 0.10.0 entries
73e3678 
Several "0.10.0" section changelog entries (see PRs #230 and #239) were
added after 0.10.0 release. This patch fixes the inconsistency by moving
them to "Unreleased" section.

Follows up #74, #237
@DifferentialOrange DifferentialOrange mentioned this pull request Apr 8, 2022
Merged
1 task
DifferentialOrange added a commit that referenced this pull request Apr 10, 2022
@DifferentialOrange
changelog: fix 0.10.0 entries
d1b1c1a 
Several "0.10.0" section changelog entries (see PRs #230 and #239) were
added after 0.10.0 release. This patch fixes the inconsistency by moving
them to "Unreleased" section.

Follows up #74, #237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Totktonada Totktonada Totktonada approved these changes

@ligurio ligurio ligurio 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.

Support custom sharding function
3 participants
@AnaNek @ligurio @Totktonada