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

Change migration progress log to be more verbose #5122

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

janezpodhostnik
Copy link
Contributor

Changed the progress log:

  • it now logs once every 10% or once every minute if the progress is slower
  • it can be called concurrently
  • it prints an eta that is estimated from the current progress and remaining work
  • log.Logger is now the first parameter
  • to add progress you call the progress logger with the difference, not the current total progress

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (fbfecc1) 56.25% compared to head (d36df28) 56.30%.
Report is 450 commits behind head on master.

Files Patch % Lines
module/util/log.go 83.33% 7 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5122      +/-   ##
==========================================
+ Coverage   56.25%   56.30%   +0.04%     
==========================================
  Files         974      976       +2     
  Lines       90742    91804    +1062     
==========================================
+ Hits        51047    51687     +640     
- Misses      35889    36286     +397     
- Partials     3806     3831      +25     
Flag Coverage Δ
unittests 56.30% <83.87%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

module/util/log.go Outdated Show resolved Hide resolved
module/util/log.go Outdated Show resolved Hide resolved
Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Looks good and logging is definitely useful during migration.

I left some comments about locking/concurrency overhead. Some are more worthwhile to optimize than others.

Maybe instead of calling logging function with every operation, we can call logging with batch of operations to reduce locking overhead that is incurred even when logging isn't sampled under the hood.

Comment on lines +51 to +53
// LogProgress takes a total and return function such that when called adds the given
// number to the progress and logs the progress every 10% or every 60 seconds whichever
// comes first.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// LogProgress takes a total and return function such that when called adds the given
// number to the progress and logs the progress every 10% or every 60 seconds whichever
// comes first.
// LogProgress takes a LogProgressConfig and return function such that when called adds the given
// number to the progress and logs the progress every specified seconds or % completed
// (whichever comes first) as specified in LogProgressConfig.

}

func (s *progressLogsSampler) Sample(lvl zerolog.Level) bool {
sample := s.basicSampler.Sample(lvl)
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe rename sample to sampled.

Suggested change
sample := s.basicSampler.Sample(lvl)
sampled := s.basicSampler.Sample(lvl)

groups, err = PayloadGrouping(groups, payload)
if err != nil {
return nil, err
}
logGrouping(i)
logGrouping(1)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe optimize by calling logGrouping() every n payloads (not every payload) because logGrouping() has locking overhead.

@@ -119,8 +130,7 @@ func MigrateGroupSequentially(
}

migrated = append(migrated, accountMigrated...)
logAccount(i)
i++
logAccount(1)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe optimize by calling logAccount() every n account migrations because logAccount() has locking overhead.

@@ -183,7 +199,7 @@ func MigrateGroupConcurrently(

accountMigrated := result.Migrated
migrated = append(migrated, accountMigrated...)
logAccount(i)
logAccount(1)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

return func(index uint64) {
lg(int(index))
lg(1)
Copy link
Member

Choose a reason for hiding this comment

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

It seems this function is called for reading and writing every node during checkpointing. Maybe check performance impact with logging and locking overhead here and optimize if needed.

@janezpodhostnik
Copy link
Contributor Author

The comments about locking will be addressed here #5140

Merged via the queue into master with commit cb152a0 Dec 13, 2023
53 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/progress-log-change branch December 13, 2023 14:23
@j1010001 j1010001 changed the title Change progress log to be more verbose Change migration progress log to be more verbose Jan 23, 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.

4 participants