implement Replica::pending_tasks

[Scandiravian]

This is a first attempt at #452.

I've implemented two different approaches in the storage layer and I'm looking for feedback on whether either of these, or a third approach, would be most optimal.

Currently the taskdb layer use the more "naive" approach, which follows the original c++ code more closely.

The alternative approach would use the get_tasks_by_status to query for pending tasks. I am however not sure whether there could potentially be an inconsistency between the working_set and the sqlite database, which would make that solution unfeasible.

[djmitche]

Nice work! I think it would be unusual to want to get all completed or deleted tasks (and those cases would be covered by all_tasks()).

And, the set of all pending tasks and the working set can be slightly different in ways that might be significant. I think the best approach here would be joining with the working set to get the working-set tasks.

[Scandiravian]

Nice work! I think it would be unusual to want to get all completed or deleted tasks (and those cases would be covered by all_tasks()).

And, the set of all pending tasks and the working set can be slightly different in ways that might be significant. I think the best approach here would be joining with the working set to get the working-set tasks.

Thanks for the feedback. I've updated the PR so it hopefully reflect your suggestions. I moved some code from pending_tasks to pending_tasks_data when implementing the latter, but the functionality should be the same.

I have also fixed the CI failures, so I think this is ready for review. Let me know if you'd like any other changes to the code

[djmitche]

Nice work! I think it would be unusual to want to get all completed or deleted tasks (and those cases would be covered by all_tasks()).
And, the set of all pending tasks and the working set can be slightly different in ways that might be significant. I think the best approach here would be joining with the working set to get the working-set tasks.

Thanks for the feedback. I've updated the PR so it hopefully reflect your suggestions. I moved some code from pending_tasks to pending_tasks_data when implementing the latter, but the functionality should be the same.

I have also fixed the CI failures, so I think this is ready for review. Let me know if you'd like any other changes to the code

This implementation looks solid and well-tested!

However, this still requires two separate queries to get the pending tasks: first to get the list of UUIDs in the pending set, and second to fetch each of those tasks. This involves sending that list of UUIDs from the DB to Rust and back. Could a single get_pending_tasks method do this in one query, with something like SELECT tasks.* from tasks join working_set on..?

[Scandiravian]

That's really good feedback. I hadn't realised that the working set already exists in the database, thanks 😄

I'm guessing that there's somewhere else in the code that ensures only tasks with the "pending" status are added to the working_set?

I've added a new commit that changes the implementation to using a join to do it in a single query. Let me know if you would like any other changes. I'm personally not entirely happy with the inmemory implementation of get_pending_tasks as it's a bit too nested/complicated making it difficult to read

If you're happy with how the implementation is looking I'm going to squash the changes into a single commit before merging and give the code another pass to see if I can improve the readability+improve the doc strings

[djmitche]

I'm guessing that there's somewhere else in the code that ensures only tasks with the "pending" status are added to the working_set?

Yes, modifications to the status property trigger adjustments of the working set.

I've added a new commit that changes the implementation to using a join to do it in a single query. Let me know if you would like any other changes. I'm personally not entirely happy with the inmemory implementation of get_pending_tasks as it's a bit too nested/complicated making it difficult to read

If you're happy with how the implementation is looking I'm going to squash the changes into a single commit before merging and give the code another pass to see if I can improve the readability+improve the doc strings

I'll have a look shortly!

[djmitche]

Really close! Just a few comments below.

[Scandiravian]

Thank you so much for the review and for taking the time to thoroughly explain the bug that my choice of LEFT JOIN would have caused - it helps me improve as a developer

I've added a commit for each comment you had to make it easier to read through. I hope it's okay to run the changes by you one final time before I squash (actually fixup) everything into a single commit so it's ready to merge 🤞

[djmitche]

Looks great, thanks! And no problem at all making another round of review.

I can squash this when merging, so if you want to just make one more commit to fix the comment, that's all that should be necessary.

[Scandiravian]

Hey @djmitche

If everything looks good to you, feel free to squash and merge the PR 😄

[djmitche]

Sorry for the delay!

[Scandiravian]

Sorry for the delay!

No problem. I wasn't sure if I had communicated clearly that I thought I had solved all remaining issues 😅

Thanks so much for all your constructive feedback on my changes. This was an incredibly pleasant experience and I hope I'll get the chance to contribute to the project again