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

Implement Delivery Optimization #909

Merged
merged 9 commits into from
Apr 30, 2021
Merged

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Apr 29, 2021

Initial work on #151

Change

This change adds code to leverage the Delivery Optimization (DO) service to download packages. Only packages are downloaded through DO at this time, as other things that we download do not fit in with the design of DO.

Currently one must still opt-in to using DO via settings, which I used rather than an experimental feature because it makes sense to allow the control long term. Eventually, we will make DO the default value here.

Settings

Two settings are added:

  1. Control over which downloader is used for packages (.network.downloader)
  2. A timeout for controlling how long to wait before DO indicates any progress has been made (.network.doProgressTimeoutInSeconds)

Validation

Only manual validation has been done currently, but when we move DO to be the default it will automatically be tested by the E2E tests.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner April 29, 2021 20:52
@JohnMcPMS JohnMcPMS changed the title Impldo Implement Delivery Optimization Apr 29, 2021
#include "Public/AppInstallerStrings.h"
#include "winget/UserSettings.h"

// Until it is in the Windows SDK, get it from here
Copy link
Contributor

Choose a reason for hiding this comment

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

Until

a TODO: for tracking?

IProgressCallback& progress,
bool computeHash = false);
bool computeHash = false,
std::string_view downloadIdentifier = {});
Copy link
Contributor

Choose a reason for hiding this comment

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

downloadIdentifier

Should we add this to the above comments? Something like, only used in DO as content id

@@ -6,6 +6,7 @@ agg
aicli
AICLIC
ajor
ALLOC
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably put these in allow.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference I know of is expect == warning and allow == no warning. But I have never seen these warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll see them if an item is no longer used and someone adds an additional misspelling.

Since these are terms, it's reasonable to move them to allow.

ajor for instance is a good example of something that should be in expect. It's there because you're currently using [M]ajor, but ajor isn't really a word, so if you rewrite [M]ajor some other way (or mask it w/ a pattern), you wouldn't want someone else to accidentally write ajor (as a typo by dropping the m).

You could for instance change:

// REG_DWORD (ex. 0xMMmmbbbb, M[ajor], m[inor], b[uild])

to:

        // REG_DWORD (ex. 0xMMmmbbbb, MM: major, mm: minor, bbbb: build)

At which point the bot would tell you it noticed ajor is no longer present (when it notices someone adding a new unrecognized word, which happens often enough).

IProgressCallback& m_progress;
std::mutex m_statusMutex;
std::condition_variable m_statusCV;
DO_DOWNLOAD_STATUS m_currentStatus{};
Copy link
Contributor

Choose a reason for hiding this comment

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

{}

= {} for code style consistency?

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@JohnMcPMS JohnMcPMS merged commit 9399b6a into microsoft:master Apr 30, 2021
@JohnMcPMS JohnMcPMS deleted the impldo branch April 30, 2021 04:46
SetProperty(DODownloadProperty_ContentId, contentId);
}

void DisplayName(std::string_view displayName)

Choose a reason for hiding this comment

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

DisplayName

Can you please add a call to this and specify a name? We suggest "MS WinGet Download".

SetProperty(DODownloadProperty_CorrelationVector, correlationVector);
}

void NoProgressTimeoutSeconds(uint32_t noProgressTimeoutSeconds)

Choose a reason for hiding this comment

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

NoProgressTimeoutSeconds

Unused. Remove it to avoid someone from introducing a call while still implementing the timeout in DODownloadStatusCallback::Wait?

Choose a reason for hiding this comment

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

Ah, I see Wait() actually waits indefinitely after the first progress update. Keep in mind DO's default timeouts are 30 minutes for foreground, 3 hours for background.

Are you ok with these defaults?

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