Update documentation of Batch data plane swagger. by xingwu1 · Pull Request #2885 · Azure/azure-rest-api-specs

xingwu1 · 2018-04-16T19:13:21Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-04-16T19:14:13Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

xingwu1 · 2018-04-16T19:15:30Z

@matthchr @darylmsft @dlepow Please review the doc change.

AutorestCI · 2018-04-16T19:15:40Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2219

AutorestCI · 2018-04-16T19:16:30Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2765

AutorestCI · 2018-04-16T19:18:04Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#1660

darylmsft · 2018-04-16T19:25:32Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

"and be": replace by "and will be" or "and"

We don't use PaaS/IaaS in public documentation.

Can you say this instead?
On CloudServiceConfiguration pools, this user is logged in with the INTERACTIVE flag. On Windows VirtualMachineConfiguration pools, this user is logged in with the BATCH flag.

We DO use PaaS/IaaS in the document, which exactly for cloudServiceConfiguration/virtualMachineConfig.

darylmsft · 2018-04-16T19:28:19Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

"All tasks should be idempotent, it means..." replace by "All tasks should be idempotent. This means tasks need to tolerate being... etc".

I don't think the email on this text and/or placement has reached a conclusion yet.

Remove the comma in "... being interrupted and restarted,without causing any corruption" #nitpick

My attempt:
"Note that this value specifically controls the number of retries for the task executable due to nonzero exit code. The Batch service will try the task once, and may then retry up to this limit. For example, if the maximum retry count is 3, Batch tries the task up to 4 times (one initial try and 3 retries). If the maximum retry count is 0, the Batch service does not retry the task after the first attempt. If the maximum retry count is -1, the Batch service retries the task without limit. Resource files and application packages are only downloaded again if the task is retried on a new compute node. Batch will also retry tasks when a recovery operation is triggered on a compute node. Examples of recovery operations include (but are not limited to) when an unhealthy compute node is rebooted or a compute node disappeared due to host failure. Retries due to recovery operations are independent of and are not counted against the maxTaskRetryCount. Even if the maxTaskRetryCount is 0, an internal retry due to a recovery operation may occur. Because of this, all tasks should be idempotent. This means tasks need to tolerate being interrupted and restarted without causing any corruption or duplicate data. Best practices recommended for long running tasks is to use checkpointing."

I think some/all of this text (specifically this bit: Batch will also retry tasks when a recovery operation is triggered on a compute node. Examples of recovery operations include (but are not limited to) when an unhealthy compute node is rebooted or a compute node disappeared due to host failure. Retries due to recovery operations are independent of and are not counted against the maxTaskRetryCount. Even if the maxTaskRetryCount is 0, an internal retry due to a recovery operation may occur. Because of this, all tasks should be idempotent. This means tasks need to tolerate being interrupted and restarted without causing any corruption or duplicate data. Best practices recommended for long running tasks is to use checkpointing. which is about internal retries should be moved to the root of the task object rather than buried down here on maxtaskRetryCount which is actually about user retries not system/internal ones.

Are we able to include new lines in the description documentation? The internal retries topic is really a separate concept than the user retries and it would be helpful to make a clear distinction when we branch off into it to prevent confusion.

I agree with Matt's point that idempotent tasks should be addressed closer to the root as well because not everyone will care enough to read into retry policy. Generally when reading documentation people will start at the base and then delve deeper into the options as needed.

hovsepm · 2018-04-16T20:10:30Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

Did you already ship client SDK with the current api version? Will they still work without this parameter being sent to server?

@hovsepm This is server response. Not request.

@xingwu1 why do you mark server response fields as required then? IMHO they should be marked as "readOnly": true to prevent client from modifying them. Required field means that those parameters should be present in the request and not response - https://swagger.io/docs/specification/describing-parameters/#required-and-optional-parameters-7

readOnly and required field can't co-exist. However, without required field, all the int properties will convert to Nullable which make user difficult to use. So right now, we will keep the required field right now. We will re-consider readonly field.

theunrepentantgeek · 2018-04-16T22:19:26Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

Remove the comma in "... being interrupted and restarted,without causing any corruption" #nitpick

theunrepentantgeek · 2018-04-16T22:20:36Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

I'm not sure that "Tasks should be idempotent" is the most important information in this description; perhaps move it to the end? (Same comment also applies elsewhere.)

Agree with Bevan.

matthchr · 2018-04-16T22:28:27Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

We don't use PaaS/IaaS in public documentation.

Can you say this instead?
On CloudServiceConfiguration pools, this user is logged in with the INTERACTIVE flag. On Windows VirtualMachineConfiguration pools, this user is logged in with the BATCH flag.

matthchr · 2018-04-16T22:45:58Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

My attempt:
"Note that this value specifically controls the number of retries for the task executable due to nonzero exit code. The Batch service will try the task once, and may then retry up to this limit. For example, if the maximum retry count is 3, Batch tries the task up to 4 times (one initial try and 3 retries). If the maximum retry count is 0, the Batch service does not retry the task after the first attempt. If the maximum retry count is -1, the Batch service retries the task without limit. Resource files and application packages are only downloaded again if the task is retried on a new compute node. Batch will also retry tasks when a recovery operation is triggered on a compute node. Examples of recovery operations include (but are not limited to) when an unhealthy compute node is rebooted or a compute node disappeared due to host failure. Retries due to recovery operations are independent of and are not counted against the maxTaskRetryCount. Even if the maxTaskRetryCount is 0, an internal retry due to a recovery operation may occur. Because of this, all tasks should be idempotent. This means tasks need to tolerate being interrupted and restarted without causing any corruption or duplicate data. Best practices recommended for long running tasks is to use checkpointing."

I think some/all of this text (specifically this bit: Batch will also retry tasks when a recovery operation is triggered on a compute node. Examples of recovery operations include (but are not limited to) when an unhealthy compute node is rebooted or a compute node disappeared due to host failure. Retries due to recovery operations are independent of and are not counted against the maxTaskRetryCount. Even if the maxTaskRetryCount is 0, an internal retry due to a recovery operation may occur. Because of this, all tasks should be idempotent. This means tasks need to tolerate being interrupted and restarted without causing any corruption or duplicate data. Best practices recommended for long running tasks is to use checkpointing. which is about internal retries should be moved to the root of the task object rather than buried down here on maxtaskRetryCount which is actually about user retries not system/internal ones.

matthchr · 2018-04-16T22:46:26Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

Avoid this style of "reference" since it doesn't look nice in any generated language. I think we need to think of a better way to do this.

This comment applies down the line.

matthchr · 2018-04-16T22:46:56Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

Agree with Bevan.

hovsepm · 2018-04-17T22:13:03Z

To all reviewers: please explicitly approve the PR then I'll be able to merge it.

azuresdkciprbot · 2018-04-18T20:27:21Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/batch/data-plane/readme.md
Before the PR: Warning(s): 1 Error(s): 0
After the PR: Warning(s): 1 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

hovsepm · 2018-04-18T21:14:42Z

Waiting for all reviewers to sign-off.

xingwu1 · 2018-04-19T16:05:14Z

@matthchr @dlepow Please review the PR and sign it off.

matthchr · 2018-04-19T20:17:42Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

Due to a nonzero exit code (missing the "a")

matthchr · 2018-04-19T20:21:31Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

If the command line refers to file paths, it should use a relative path (relative to the task working directory), or use the Batch provided environment variables (https://docs.microsoft.com/en-us/azure/batch/batch-compute-node-environment-variables)

This applies to all copies of this description, since there are a few

matthchr · 2018-04-19T20:24:35Z

specification/batch/data-plane/Microsoft.Batch/stable/2018-03-01.6.1/BatchService.json

At the end:
"The best practice for long running tasks is to use some form of checkpointing."

Applies to all copies

azuresdkciprbot · 2018-04-19T21:13:41Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/batch/data-plane/readme.md
Before the PR: Warning(s): 1 Error(s): 0
After the PR: Warning(s): 1 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

xingwu1 · 2018-04-19T21:31:38Z

@hovsepm you can merge it

azuresdkci assigned hovsepm Apr 16, 2018

azuresdkci requested a review from hovsepm April 16, 2018 19:13

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR batch/data-plane] Update documentation of Batch data plane swagger. Azure/azure-sdk-for-python#2406

Merged

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR batch/data-plane] Update documentation of Batch data plane swagger. Azure/azure-sdk-for-node#2748

Merged

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR batch/data-plane] Update documentation of Batch data plane swagger. Azure/azure-sdk-for-go#1607

Closed

darylmsft reviewed Apr 16, 2018

View reviewed changes

hovsepm reviewed Apr 16, 2018

View reviewed changes

theunrepentantgeek reviewed Apr 16, 2018

View reviewed changes

matthchr requested changes Apr 16, 2018

View reviewed changes

xingwu1 force-pushed the master branch from 5670fe8 to 9a3132b Compare April 16, 2018 23:31

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR batch/data-plane] Update documentation of Batch data plane swagger. Azure/azure-sdk-for-go#1621

Merged

xingwu1 force-pushed the master branch from 9a3132b to b757936 Compare April 18, 2018 20:22

hovsepm approved these changes Apr 18, 2018

View reviewed changes

matthchr requested changes Apr 19, 2018

View reviewed changes

Update documentation of Batch data plane swagger.

952daf5

xingwu1 force-pushed the master branch from b757936 to 952daf5 Compare April 19, 2018 21:08

matthchr approved these changes Apr 19, 2018

View reviewed changes

hovsepm merged commit 9d16433 into Azure:master Apr 19, 2018

matthchr mentioned this pull request Apr 26, 2018

Azure Batch C# SDK version 8.1.1 Azure/azure-sdk-for-net#4270

Merged

9 tasks

Conversation

xingwu1 commented Apr 16, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-libraries-for-java

Uh oh!

xingwu1 commented Apr 16, 2018

Uh oh!

AutorestCI commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm commented Apr 17, 2018

Uh oh!

azuresdkciprbot commented Apr 18, 2018

Uh oh!

hovsepm commented Apr 18, 2018

Uh oh!

xingwu1 commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingwu1 commented Apr 16, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

hovsepm Apr 17, 2018 •

edited

Loading