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

Rework the command-line interface #292

Merged
merged 16 commits into from
Jan 14, 2020
Merged

Rework the command-line interface #292

merged 16 commits into from
Jan 14, 2020

Conversation

derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Jan 10, 2020

This PR is probably too large and I'll need to split it into smaller parts. However, it has the full story of how to redo our command-line interface:

  • Commands clone, diagnose, config, cache-server are untouched.

  • The repos command is dropped in favor of register, list, and unregister. Resolves ReposVerb: alternate names for add/remove #286.

  • Created the pause and resume commands. Starts New Feature: pause maintenance #284.

  • Replaced remove with delete. Resolves Make RemoveVerb a subset of ReposVerb #274.

  • Reorganized the docs folder. Instead of a command reference page for each command, commands are grouped by purpose and documented together. These sections are:

    • Getting Started: clone, register, unregister, delete.
    • Advanced: run, pause, resume.
    • Troubleshooting: upgrade, diagnose, config, cache-server.

Not done yet, but can be done in a later PR:

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Copy link
Member

@jrbriggs jrbriggs 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. A few minor points.

Scalar.Service/MaintenanceTaskScheduler.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/DeleteVerb.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/DeleteVerb.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/PauseVerb.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/RegisterVerb.cs Outdated Show resolved Hide resolved
docs/getting-started.md Outdated Show resolved Hide resolved
docs/troubleshooting.md Outdated Show resolved Hide resolved
docs/troubleshooting.md Show resolved Hide resolved
docs/troubleshooting.md Outdated Show resolved Hide resolved
docs/troubleshooting.md Outdated Show resolved Hide resolved
Copy link
Member

@kewillford kewillford left a comment

Choose a reason for hiding this comment

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

LGTM a few minor comments.


private string GetMaintenanceDelayFilePath()
{
return Path.Combine(this.registryFolderPath, "maintenance-pause.time");
Copy link
Member

Choose a reason for hiding this comment

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

Why use a .time extension? My personal preference would be to use the name of maintenance-pause-time and a .txt extension or no extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what we are using for the loose object step and packfile maintenance step.

new PhysicalFileSystem(),
repoRegistryLocation);

if (!repoRegistry.TryPauseMaintenanceUntil(pauseTime, out string error))
Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud here and what do we expect if there are tasks running when someone runs pause? Are they going to expect that nothing is running now and they can do some work without them running or will they know that there could be some tasks running that would need to finish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. Do we expect those steps to stop? If there are multiple enlistments, then should it stop during the "for each enlistment" loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added details to #284 with these questions.

Scalar/CommandLine/ScalarVerb.cs Show resolved Hide resolved
Scalar/CommandLine/UnregisterVerb.cs Outdated Show resolved Hide resolved
docs/advanced.md Outdated Show resolved Hide resolved
docs/advanced.md Outdated Show resolved Hide resolved
Co-Authored-By: Kevin Willford <kewillf@microsoft.com>
Co-Authored-By: John Briggs <jobriggs@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee merged commit ca1b26a into microsoft:master Jan 14, 2020
derrickstolee added a commit that referenced this pull request Jan 23, 2020
…task

The `PauseVerb` was added along with a lot of other things in #292. It creates a file storing a timestamp for marking how long to pause maintenance. As noted in #284, this was inadequate. If there are a lot of enlistments and a user runs `scalar pause` in the middle of iterating over those enlistments, the maintenance task will continue to run over all of those enlistments.

Now, stop before triggering a new `scalar run <task>` process. Modify the markdown with the details for how many maintenance tasks completed and whether or not it was halted due to a pause. Here is a formatted version of a trace event for a task during pause:

```
[2020-01-22 17:17:57.7293 -05:00] RunMaintenanceTaskForRepos_MaintenanceSummary 
{
"task":"loose-objects",
"UserId":"S-1-5-21-4082248826-4152932973-4176629329-1001",
"SessionId":1,
"traceMessage":"Maintenance is paused until 1/23/2020 4:14:44 AM.",
"reposInRegistryForUser":11,
"reposSkipped":11,
"reposSuccessfullyRemoved":0,
"repoRemovalFailures":0,
"reposMaintained":0,
"haltedForPause":true
}
```
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.

ReposVerb: alternate names for add/remove Make RemoveVerb a subset of ReposVerb
3 participants