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

Associate the harmony_task_history table with the sector #149

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

strahe
Copy link
Contributor

@strahe strahe commented Aug 14, 2024

No description provided.

@strahe strahe marked this pull request as draft August 14, 2024 09:52
Copy link
Collaborator

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

Thanks a lot for this; I'd avoid the msgpack dependency, I don't think the complexity is worth the minimal savings it can get us, and it makes querying details much harder

harmony/harmonydb/sql/20240814-sectors-task-events.sql Outdated Show resolved Hide resolved
harmony/harmonydb/sql/20240814-sectors-task-events.sql Outdated Show resolved Hide resolved
harmony/harmonydb/sql/20240814-sectors-task-events.sql Outdated Show resolved Hide resolved
@strahe strahe changed the title Add table to record sector task events Associate the harmony_task_history table with the sector. Aug 15, 2024
@strahe strahe changed the title Associate the harmony_task_history table with the sector. Associate the harmony_task_history table with the sector Aug 15, 2024
@LexLuthr
Copy link
Contributor

I would not recommend this approach at all. Changing the harmonyTask to be sector specific is not a good idea. HarmonyTask is generic for a reason. This would thrown a wrench in our plans to later do market, PDP and other things. A separate Event table would be far better. Harmony history should have nothing to do with sector events.

@strahe
Copy link
Contributor Author

strahe commented Aug 18, 2024

I removed the fields added to history table and used the new event table to record the history IDs.
It looks like:
图片

图片

Copy link
Collaborator

@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 looks good now, I'd still improve 2 small things, especially the schema change which will be hard to change after this lands

harmony/harmonydb/sql/20240814-pipeline-task-events.sql Outdated Show resolved Hide resolved
tasks/seal/task_porep.go Show resolved Hide resolved
Co-authored-by: Łukasz Magiera <magik6k@users.noreply.github.com>
Copy link
Contributor

@LexLuthr LexLuthr left a comment

Choose a reason for hiding this comment

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

I think we should record events regardless of failure or success. If we only record success, chances are we will never actually use them. Why would you check logs for working system? We should record all failures as well.

We also need some kind of cleanup mechanism for this table. It will grow very quickly on large clusters.

@strahe
Copy link
Contributor Author

strahe commented Aug 19, 2024

@LexLuthr Totally agree. We manage over 100 miners, so the data volume could become very large, but I'm afraid that too many changes will make this PR increasingly complex.😀I will keep records of successes and failures in this PR.

@LexLuthr
Copy link
Contributor

I would say this is still a relatively smaller PR. We can add cleanup in this and mark this as a feature with documentation on how to get these events from CLI.

@strahe
Copy link
Contributor Author

strahe commented Aug 20, 2024

I think we should record events regardless of failure or success. If we only record success, chances are we will never actually use them. Why would you check logs for working system? We should record all failures as well.

We also need some kind of cleanup mechanism for this table. It will grow very quickly on large clusters.

I confirmed again that the history table records failed entries, so the event table will also default to recording failed entries. Regarding data growth, as Magik6k mentioned, SST compression might be helpful. It seems that the history table will impose a greater load compared to the event table.

@strahe
Copy link
Contributor Author

strahe commented Aug 21, 2024

List events:

image

# List all events
curio seal events               

# List all events for the specified actor
curio seal events --actor actor  

# List events for the specified sector and actor
curio seal events --actor actor --sector sector  

@strahe strahe marked this pull request as ready for review August 21, 2024 07:03
Copy link
Collaborator

@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 extremely useful, thanks for the PR!

tasks/seal/task_porep.go Show resolved Hide resolved
harmony/harmonydb/sql/20240814-sectors-task-events.sql Outdated Show resolved Hide resolved
@magik6k magik6k merged commit 555a695 into filecoin-project:main Aug 21, 2024
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.

3 participants