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

downloader: small refactoring #13338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

downloader: small refactoring #13338

wants to merge 2 commits into from

Conversation

dvovk
Copy link
Member

@dvovk dvovk commented Jan 7, 2025

Move code which calculates rates for web downloader client to separate function.

@dvovk dvovk requested review from mh0lt and AskAlexSharov and removed request for mh0lt January 7, 2025 07:46
@@ -2968,3 +2904,71 @@ func (d *Downloader) CompletedTorrents() map[string]completedTorrentInfo {

return d.completedTorrents
}

func (d *Downloader) collectWebDownloadClientStats(downloading *map[string]*downloadInfo, stats *AggStats, zeroProgress *[]string, webTransfers *int32) {
Copy link
Member

@awskii awskii Jan 9, 2025

Choose a reason for hiding this comment

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

webTransfers, zeroProgress could be a return value instead of passed by ptr.
Of course zero progress list should be appended to existing one but would be cleaner

Copy link
Member Author

Choose a reason for hiding this comment

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

It make sense to do it

tLen := transfer.Size
transferName := transfer.Name

delete(*downloading, transferName)
Copy link
Member

Choose a reason for hiding this comment

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

i know before we also had a deletion here but SHOULD we really delete file name here? webStats.Transferring supposes transfer is not over yet so why need to delete it from downloading map?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it is looks not logical but the issue is that we don't use web download client(WDL) and this code is just sitting here without usage so that is why I decided to move it into the separate function. Maybe it even worth to delete it completely as I will not spend time to setup, test, etc... for WDL and I don't think that anyone else will do it in near future.

}
}

*webTransfers += int32(len(*downloading))
Copy link
Member

Choose a reason for hiding this comment

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

so should we add size of downloading list after we counted each transfer and removed filename from downloading list?

Copy link
Member Author

Choose a reason for hiding this comment

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

see comment above

@awskii
Copy link
Member

awskii commented Jan 9, 2025

another approach could be AggStat object delta which should be added to existing aggStat. So different modules return new AggStat with non zero fields only where needed and then we Add those stats to main statistics object, which also easier to maintain and read.

@dvovk dvovk requested a review from awskii January 10, 2025 14:53
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.

2 participants