Updated mpb bar progress bars #132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @joereuss12, it looks like something that was changed is causing the Test action to throw this: Can you investigate and let me know when you've patched things back up? |
|
@jhiemstrawisc just updated with some more changes too. I can undo the log level if needed depending on how Brian responds to the issue I made (#135) and I'll make a note in the issue to revert it back if needed. |
bbockelm
requested changes
Sep 16, 2023
cmd/object_copy.go
Outdated
| @@ -77,7 +77,7 @@ func copyMain(cmd *cobra.Command, args []string) { | |||
| if val, err := cmd.Flags().GetBool("debug"); err == nil && val { | |||
| setLogging(log.DebugLevel) | |||
| } else { | |||
| setLogging(log.ErrorLevel) | |||
There was a problem hiding this comment.
NACK on changing the error behavior as part of an unrelated PR.
handle_http.go
Outdated
| progressBar.DecoratorEwmaUpdate(currentCompletedTime.Sub(previousCompletedTime)) | ||
| previousCompletedTime = currentCompletedTime | ||
| start := time.Now() | ||
| progressBar.EwmaIncrement(time.Since(start)) |
There was a problem hiding this comment.
How is this supposed to work? The time.Since is always, more-or-less, going to be zero here, right? How is it measuring progress?
handle_http.go
Outdated
| startBelowLimit = time.Now().Unix() | ||
| continue | ||
| } else if (time.Now().Unix() - startBelowLimit) < slowTransferWindow { | ||
| } else if (time.Now().Unix() - startBelowLimit) < int64(slowTransferWindow) { |
handle_http.go
Outdated
| progressBar.Abort(true) | ||
| log.Errorln("cancelled, too slow!") | ||
| log.Info("retrying...") | ||
| //cancelledProgressBar.SetTotal(resp.Size, true) |
There was a problem hiding this comment.
Isn't there a higher function that emits an error message? Seems strange to do that from the progress bar.
This fixes a bug when downloading from an origin for the first time and a weird error message shows up. Turns out the package needed some updating. This took some time since the function we originally had for v7 no longer existed in v8. Also changed around the styling a tad and made it look nice and work with this new version upgrade. Removed what I thought was an unnecessary extra bar being made to say "done". Also found a typo to be fixed within handle_http
Updated the code so progress bars work with tests. Also made a few more changes: 1. Updated messages/bar when download is taking too long, didn't see a point of making a whole new bar. Have a log.error saying "cancelled" and info saying "retrying" as well as abort the bar 2. Changed the log level for `object copy` to be log.Info, thought a more descriptive log would be good for users and is not displaying too much more info to where it would be annoying.
Updated progress bars per requests from PR. Made measuring progress actually work now (wasn't before) by calculating correct time and bytes that have progressed. Fixed a few leftover typos and changed up error messages to appear cleaner and give better worded info. The only issue: I could not get a logrus warning message to print with the progress bars. The bar would quickly override the message and would ususally end up outputting 2 bars instead of 1. My current fix: There is a function in the mpb package that allows you to write above the progress bars (but has to be a byte string). I could not figure out how to convert a logrus warning to a byte string to be outputted (if there even is a way). Therefore, I created a little warning to appear over the progress bar if the download is happening too slow. Even though it is not a logrus warning, I feel still helpful to know.
bbockelm
force-pushed
the
update-progress-bars-branch
branch
from
6186a3e to
50d782b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug when downloading from an origin for the first time and a weird error message shows up. Turns out the package needed some updating. This took some time since the function we originally had for v7 no longer existed in v8. Also changed around the styling a tad and made it look nice and work with this new version upgrade. Removed what I thought was an unnecessary extra bar being made to say "done". Also found a typo to be fixed within handle_http.