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

Add ability to pass args to start_key function #114

Open
ArtDu opened this issue Jun 21, 2022 · 2 comments
Open

Add ability to pass args to start_key function #114

ArtDu opened this issue Jun 21, 2022 · 2 comments
Labels
1sp good first issue Good for newcomers

Comments

@ArtDu
Copy link
Contributor

ArtDu commented Jun 21, 2022

expirationd/expirationd.lua

Lines 691 to 698 in c3b86c8

-- check start_key
if options.start_key ~= nil or options.start_key == box.NULL then
if type(options.start_key) == "function" then
task.start_key = function() return options.start_key() end
else
task.start_key = function() return options.start_key end
end
end

expirationd/expirationd.lua

Lines 382 to 390 in c3b86c8

-- default iterate_with function
local function default_iterate_with(task)
return task.index:pairs(task.start_key(), { iterator = task.iterator_type })
:take_while(
function()
return task:process_while()
end
)
end

Should be rewritten to:

-- check start_key 
 if options.start_key ~= nil or options.start_key == box.NULL then 
     if type(options.start_key) == "function" then 
         task.start_key = function(...) return options.start_key(...) end 
     else 
         task.start_key = function() return options.start_key end 
     end 
 end 
-- default iterate_with function
local function default_iterate_with(task)
    return task.index:pairs(task.start_key(task.args), { iterator = task.iterator_type })
       :take_while(
            function()
                return task:process_while()
            end
        )
end

Or should we pass task as it says in README?

expirationd/README.md

Lines 94 to 129 in c3b86c8

```lua
expirationd.start(job_name, space.id, is_expired, {
-- name or id of the index in the specified space to iterate over
index = "exp",
-- one transaction per batch
-- default is false
atomic_iteration = true,
-- delete data that was added a year ago
-- default is nil
start_key = function( task )
return clock.time() - (365*24*60*60)
end,
-- delete it from the oldest to the newest
-- default is ALL
iterator_type = "GE",
-- stop full_scan if delete a lot
-- returns true by default
process_while = function( task )
if task.args.max_expired_tuples >= task.expired_tuples_count then
task.expired_tuples_count = 0
return false
end
return true
end,
-- this function must return an iterator over the tuples
iterate_with = function( task )
return task.index:pairs({ task.start_key() }, { iterator = task.iterator_type })
:take_while( function( tuple )
return task:process_while()
end )
end,
args = {
max_expired_tuples = 1000
}
})
```

@oleg-jukovec
Copy link
Contributor

oleg-jukovec commented Jun 24, 2022

Thank you for the issue!

It seems to me that we should to fix the README. I don't see a real case of usage the task argument in the start_key function considering that we only provide to an user the interface:

task.start (self)
task.stop (self)
task.restart (self)
task.kill (self)
task.statistics (self)

and the user should not use other fields or methods:

expirationd/expirationd.lua

Lines 210 to 213 in c3b86c8

-- NOTE: task object contains a number of properties that available for users.
-- However these properties are not a part of expirationd API. Property name
-- can be changed or property itself can be removed in future version. Be
-- careful!

@oleg-jukovec oleg-jukovec added the good first issue Good for newcomers label Jul 19, 2022
@ArtDu
Copy link
Contributor Author

ArtDu commented Apr 5, 2023

Thank you for the issue!

It seems to me that we should to fix the README. I don't see a real case of usage the task argument in the start_key function considering that we only provide to an user the interface:

task.start (self)
task.stop (self)
task.restart (self)
task.kill (self)
task.statistics (self)

and the user should not use other fields or methods:

expirationd/expirationd.lua

Lines 210 to 213 in c3b86c8

-- NOTE: task object contains a number of properties that available for users.
-- However these properties are not a part of expirationd API. Property name
-- can be changed or property itself can be removed in future version. Be
-- careful!

We can pass args instead task. It's useful when you want to start iteration from point and you depends on user data(args).
For example, if I wanted to remove by time with some threshold I would use start_key

function get_lower_bound(args)
    return fiber.time() - args.threshold
end

expirationd.start(name, space, is_expired, {
  index = 'created_at_index',
  start_key = get_lower_bound,
  iterator_type = "LT"
  args = {
    threshold = 30 -- seconds
  }
})

And we've already had the flexibility of user callbacks which have task as argument

task.on_full_scan_start(task)
local state, err = pcall(task.do_worker_iteration, task)
-- Following functions are on_full_scan*,
-- but we probably did not complete the full scan,
-- so we should check for cancellation here.
if task.worker_cancelled then
fiber.self():cancel()
end
if state then
task.on_full_scan_success(task)
else
task.on_full_scan_error(task, err)

That's why I prefer to pass task for having consistent with others callbacks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1sp good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants