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

Features for more customized behavior #52

Merged
merged 7 commits into from
Jul 6, 2021
Merged

Features for more customized behavior #52

merged 7 commits into from
Jul 6, 2021

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Apr 20, 2021

  • Iterations over Secondary Indexes - name of the index to iterate on
  • Transaction support - ability to process tuples from each batch in a single transaction
  • Start key - start iterating from the tuple with this index value
  • Iterator type - type of the iterator to use, as string or box.index constant
  • Process while function - function to call before checking each tuple, if it returns false, the task will stop until next full scan
  • And the ability to write custom iterator behavior for the user - iterate_with function which returns an iterator object which provides tuples to check

Additional changes:
Rewritten space iteration for tree index from select to pairs since the iterator is now stable
Added some comments and removed duplicate code(expirationd_kill_task)
Described new parameters in the code

Test coverage added.

RFC: #50

@ArtDu ArtDu force-pushed the indexpiration branch 6 times, most recently from 9735c15 to 1b3f665 Compare April 22, 2021 14:53
expirationd.lua Outdated Show resolved Hide resolved
expirationd.lua Outdated Show resolved Hide resolved
@ArtDu ArtDu force-pushed the indexpiration branch 17 times, most recently from cd76247 to 8f9cce7 Compare April 30, 2021 15:54
@ArtDu ArtDu force-pushed the indexpiration branch 2 times, most recently from 665c9a8 to 89106b1 Compare May 11, 2021 09:50
@ArtDu ArtDu force-pushed the indexpiration branch 2 times, most recently from 82923e6 to 6858181 Compare June 30, 2021 22:45
This was referenced Jul 1, 2021
@ArtDu ArtDu requested a review from olegrok July 1, 2021 15:14
Copy link

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

I give up. Probably we should start to file issues for some comments. Otherwise there is a risk that this patch never will be merged.

Makefile Show resolved Hide resolved
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.

  • Limit commit message width in commits "Iterate_with and process_while" and "Start key and iterator type"
  • We decided to move some requested on review changes to the backlog. Please make sure all appropriated tickets submitted.
  • We need to enable new tests as soon as you added them. So put luatest related changes to commit
    where first test is appeared (it is "Iterate over any index").
  • Changes are not atomic now. I see commits with new functionality without any documentation in a README.
    If we don't need to document these changes then please explain why.
  • Why do you mix double quotes and single quotes in a whole patchset?
  • Latest commit should contain a "Closes RFC: Adding Iterations over Secondary Indexes  #50", otherwise it is not clear finished task or not.

@ArtDu ArtDu force-pushed the indexpiration branch 2 times, most recently from 9822de0 to 914af20 Compare July 3, 2021 15:52
@ArtDu
Copy link
Contributor Author

ArtDu commented Jul 5, 2021

  • Limit commit message width in commits "Iterate_with and process_while" and "Start key and iterator type"

  • We decided to move some requested on review changes to the backlog. Please make sure all appropriated tickets submitted.

  • We need to enable new tests as soon as you added them. So put luatest related changes to commit
    where first test is appeared (it is "Iterate over any index").

  • Changes are not atomic now. I see commits with new functionality without any documentation in a README.
    If we don't need to document these changes then please explain why.

  • Why do you mix double quotes and single quotes in a whole patchset?

  • Latest commit should contain a "Closes RFC: Adding Iterations over Secondary Indexes  #50", otherwise it is not clear finished task or not.

  • Reduced the body size of all commits to 72 characters.
  • Luatest is added to ci with the first appearance of tests ("Iterate over any index" commit).
  • Changes are atomic now. Each commit on changing the functionality contains the addition of functionality, its description in the readme and testing it in the luatest.
  • Mix double quotes and single quotes were not in the code. The outer ones are double quotes, the same style has been added to the README for greater consistency.
  • Added closes: RFC: Adding Iterations over Secondary Indexes  #50 in last commit body message.

@ArtDu ArtDu requested a review from ligurio July 5, 2021 07:56
README.md Outdated Show resolved Hide resolved
expirationd.lua Show resolved Hide resolved
expirationd.lua Outdated Show resolved Hide resolved
.github/workflows/fast_testing.yaml Show resolved Hide resolved
Makefile Show resolved Hide resolved
ArtDu added 7 commits July 5, 2021 13:06
Remove expirationd_kill_task, duplicate of code.
Comments, readme and responses to errors are presented
in a more uniform form. Added additional comments for
easier understanding of what is happening. Delete `...`,
can't be jitted. Using outer double quotes only.

Needed for: #50
The vinyl and memtx tree index have the same iteration logic using
pairs. This is confirmed by the stability of iterators, see
tarantool/doc#2102

Needed for: #50
Added the ability to iterate over any index by specifying the index name
in options. The default is primary index.

ci: installation, caching and running luatest

PHONY added to Makefile as makefile target
and luatests folder are the same.

Needed for: #50
Added the ability from where to start the iterator
and the type of the iterator itself. Start key can
be set as a function (dynamic parameter) or just a
static value. The type of the iterator can be specified
either with the box.index.* constant,
or with the name for example, 'EQ' or box.index.EQ

Needed for: #50
For more flexible functionality, added the ability
to create a custom iterator that will be created at
the selected index (iterate_with). You can also pass a
predicate that will stop the fullscan process,
if required(process_while).

Needed for: #50
One transaction per batch option.
With task:kill, the batch with transactions will be finalized and only
after that the fiber will complete its work

Needed for: #50
Added an example of how to use the new features for the user.
Description of how to run luatests. Run luatest via Makefile with tap tests

Closes: #50
@ligurio ligurio merged commit 88653b3 into master Jul 6, 2021
@ligurio ligurio deleted the indexpiration branch July 6, 2021 07:53
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.

5 participants