-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add APIs for tumbling window rerun trigger #4060
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
Conversation
Automation for azure-sdk-for-jsNothing to generate for azure-sdk-for-js |
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
|
Can one of the admins verify this patch? |
dsgouda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a brand new service endpoint, needs an ARM review.
Terminal status codes for long running operations in particular look a little iffy
| "$ref": "./examples/RerunTriggers_Create.json" | ||
| } | ||
| }, | ||
| "description": "Creates?a?rerun?trigger.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the descriptions (is there a reason why ? is used instead of space)
| } | ||
| }, | ||
| "nextLink": { | ||
| "description": "The continuation token for getting the next page of results, if any remaining results exist, null otherwise.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mark this as readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done
|
Automation for azure-sdk-for-go fails with error "fatal: git fetch-pack: expected ACK/NAK, got '?error: Access denied to IP 104.42.78.222' ". How could I re-run this check? |
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/RerunTumblingWindowTriggerActionParameters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant the same model which is used for response be used here in the request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of RerunTriggerResponse is different than existing response, it's ref of RerunTriggerResource and property is RerunTumblingWindowTrigger
| } | ||
| } | ||
| }, | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/triggers/{triggerName}/rerunTriggers/{rerunTriggerName}": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the "reruntriggers" resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What functionality/scenario does it address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rerun trigger addresses the requirement from customers who want to re-run existing trigger past trigger runs, in case if old input data is corrupted and etc. For example, customer created a trigger starting from 6/1/2018-12/31/2018. If the trigger started and ran for a week, then customer found input data is partially missing, and wanted to reprocess this week of data, he would use this function and create a re-run trigger for 6/12018-6/7/2018.
|
@jhendrixMSFT Could you take a look at why the Go build is failing |
|
@AutorestCI regenerate azure-sdk-for-go |
|
@dsgouda it looks like it's a transient error. I asked the bot to regenerate the PR but it seems to be running slow (I have several other PRs still waiting on Go SDK codegen from the bot). |
|
Looks good. Signing off from ARM side. |
dsgouda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good apart from a minor comment
| "$ref": "#/parameters/triggerName" | ||
| }, | ||
| { | ||
| "$ref": "#/parameters/api-version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please stick to camelCase convention for the parameter name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is legacy issue. api-version is defined under parameters for a long time, and all the other APIs refer this parameter as api-version. If we update api-version name, then all APIs using it need to be updated too. Do you think it's still necessary to update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can skip this
|
@AutorestCI regenerate azure-sdk-for-go |
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
api-versionin the path should match theapi-versionin the spec).Quality of Swagger