fix(migration): improve job progress logging in MigrateStateTree #329
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #329 +/- ##
=======================================
Coverage 3.39% 3.39%
=======================================
Files 662 662
Lines 177818 177816 -2
=======================================
Hits 6041 6041
+ Misses 169928 169926 -2
Partials 1849 1849
|
|
@virajbhartiya do this to After each network upgrade, one of the tasks performed is that someone (usually @rjan90) opens a PR here that copies the previous actors to a new version and then copies the most basic migration, without any of the custom stuff that was needed for that last network upgrade. You can see that for v16 here: #325 |
|
Actually, the later migrations run through |
builtin/v9/migration/top.go
Outdated
| percentComplete := float64(doneNow) / float64(jobsNow) * 100 | ||
|
|
||
| log.Log(rt.INFO, "Performing migration: %d of %d jobs complete (%.1f%%, %.0f/s)", | ||
| doneNow, jobsNow, percentComplete, rate) |
There was a problem hiding this comment.
When you watch a migration happen using lotus-shed you might find that percentComplete is not helpful and we need to remove it. I hoped we could do this and my original issue I think said we should, but watching it progress myself we are adding jobs at the same time as completing them, so we never know what "complete" is. I don't think we have an easy way to pre-calculate the number of jobs. So, unfortunately, the main benefit of this logging is simply letting people know that it's happening and that their node isn't stuck.
There was a problem hiding this comment.
Will something like this work
log.Log(rt.INFO, "Performing migration: %s of %s jobs processed (%s/s) [%v elapsed]", doneStr, jobsStr, rateStr, elapsed.Round(time.Second))even though the done and total jobs will be changing, it will still show that the node isn't stuck and working just fine.
migration/runner.go
Outdated
| jobsStr := fmt.Sprintf("%d", jobsNow) | ||
| doneStr := fmt.Sprintf("%d", doneNow) | ||
| rateStr := fmt.Sprintf("%.0f", rate) | ||
|
|
||
| log.Log(rt.INFO, "Performing migration: %s of %s jobs processed (%s/s) [%v elapsed]", | ||
| doneStr, jobsStr, rateStr, elapsed.Round(time.Second)) |
There was a problem hiding this comment.
PR is looking good @virajbhartiya but why do we have the separate fmt.Sprintf's for the numbers and not just using the log.Log for that? If we're not pulling apart the Log call manually in lotus then they can be arbitrary values so numbers should be fine, no?
There was a problem hiding this comment.
Oh right that could be done directly, thanks!, updated it
reference to filecoin-project/lotus#12732
This pull request includes an update to the logging format in the
MigrateStateTreefunction to improve clarity during the migration process.Logging improvements:
builtin/v9/migration/top.go: Changed the log message to include the percentage of jobs completed and rephrased the message for better readability.