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

feat:miner:harmonytask #11165

Merged
merged 17 commits into from
Aug 25, 2023
Merged

feat:miner:harmonytask #11165

merged 17 commits into from
Aug 25, 2023

Conversation

snadrus
Copy link
Collaborator

@snadrus snadrus commented Aug 14, 2023

Related Issues

HarmonyTask implementation (PR attempt 2)

Proposed Changes

HarmonyTasks package and Resources package to manage tasks.

Additional Info

Tasks are listed in Yugabyte and competed-for through a greedy (but not too greedy) algorithm that enables tasks to hop from creator-machine to consumer machine or even from task-completion-machine to following-task-defined-elsewhere-machine.
See doc.go
Overview in https://docs.google.com/document/d/1lPe-6HdVe_oU0mD29f9Zi4233_xhSbyiXxM_6zXG4hY/edit#heading=h.k4h9btwn9p1y

@snadrus snadrus requested a review from a team as a code owner August 14, 2023 16:40
@magik6k
Copy link
Contributor

magik6k commented Aug 14, 2023

The other PR: #11108

@snadrus
Copy link
Collaborator Author

snadrus commented Aug 16, 2023

@magik6k Please review this one. The other somehow pulled-in feat/sturdypost so was abandoned.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

This is really starting to look like code that is going to be actually in-use in production in near-ish future. Couple of comments, and definitely can't wait to see stuff implemented on top

lib/harmony/harmonytask/doc.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/taskTypeHandler.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/taskTypeHandler.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/resources/resources.go Show resolved Hide resolved
lib/harmony/resources/resources.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/taskTypeHandler.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/resources/resources.go Outdated Show resolved Hide resolved
@snadrus
Copy link
Collaborator Author

snadrus commented Aug 21, 2023

Please review again. The def_test (test_unit_node) issue is something Shrenuj has addressed elsewhere (it's in the dest branch). I don't think the extern issue (test_unit_rest) is from this work.

@shrenujbansal
Copy link
Contributor

Looking at the wdpost task generation logic, I'm a little concerned that the Task Interface framework doesn't quite fit the bill and there has to be a non significant amount of work done to make it fit

@shrenujbansal
Copy link
Contributor

How was the case with concurrent tasks inserting and taking a task tested?

@shrenujbansal
Copy link
Contributor

shrenujbansal commented Aug 24, 2023

I'm happy to just get this merged in our branch since this is additive code and iterate further on it as we see fit. Unless @magik6k wants to see any more changes to this

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

First pass, I feel like I'm starting to get a pretty good intuition of how this works.

There will probably be a number of edge-cases that we will discover as we exercise this logic, but to do that we need to start actually building on top of it.

cmd/lotus-worker/main.go Outdated Show resolved Hide resolved
itests/harmonytask_test.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/doc.go Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Most places use snake_case for naming files, so this could be task_type_handler.go

lib/harmony/resources/miniopencl/miniopencl.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/harmonytask.go Outdated Show resolved Hide resolved
lib/harmony/harmonytask/taskTypeHandler.go Outdated Show resolved Hide resolved
@rjan90 rjan90 linked an issue Aug 25, 2023 that may be closed by this pull request
@snadrus snadrus merged commit f2a90ae into feat/sturdypost Aug 25, 2023
@snadrus snadrus deleted the feat/harmonytask branch August 25, 2023 23:15
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.

Feat: deps: LotusTasks package for HA
3 participants