Skip to content

[Azure Stream Analytics] Add Power BI and Azure Data Lake Store output support#1546

Merged
olydis merged 2 commits intoAzure:currentfrom
atpham256:current
Aug 30, 2017
Merged

[Azure Stream Analytics] Add Power BI and Azure Data Lake Store output support#1546
olydis merged 2 commits intoAzure:currentfrom
atpham256:current

Conversation

@atpham256
Copy link
Copy Markdown
Contributor

@atpham256 atpham256 commented Aug 16, 2017

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

- Updated swagger spec to include support for Power BI and Azure Data
Lake Store outputs
- Added/updated x-ms-examples for PUT, PATCH, and GET requests with
regards to outputs
- Updated some descriptions to be more accurate
@msftclas
Copy link
Copy Markdown

@atpham256,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@atpham256
Copy link
Copy Markdown
Contributor Author

Ran validation tools and there were no new errors compared to the previous time I ran it for the last PR.

The OBJECT_ADDITIONAL_PROPERTIES errors for oav validate-example seems to be a bug on the tool where it cannot handle response bodies with polymorphic response object models which was also present in my last PR. The examples are definitely valid in this case.

@azuresdkciprbot
Copy link
Copy Markdown

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/streamanalytics/resource-manager/readme.md
Before the PR: Warning(s): 4 Error(s): 5
After the PR: Warning(s): 4 Error(s): 5

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

@olydis olydis added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 16, 2017
@olydis
Copy link
Copy Markdown
Contributor

olydis commented Aug 16, 2017

@ravbhatnagar Breaking change in existing API version (addition of new type in polymorphic hierarchy that can be returned from server => deserialization can fail in previously generated artifacts)

@olydis
Copy link
Copy Markdown
Contributor

olydis commented Aug 21, 2017

@ravbhatnagar ping

@ravbhatnagar
Copy link
Copy Markdown
Contributor

@atpham256 - The service seems to be supporting a new model type (PowerBIOutput). Please update the api-version for this change since this is a breaking change.

@atpham256
Copy link
Copy Markdown
Contributor Author

@ravbhatnagar Hi Gaurav, this is an additive change that we do not consider a breaking change for our customers. From our talks with other teams such as ADF, they seem to be following a similar model of not considering additive changes as breaking changes. We have been using this model for a while and have not had any complaints from our customers about additive changes. Also, this has also been exposed in our service for a while (if you look at Portal, you can create a Power BI or ADLS output for ASA jobs).

Having to add a new API version for each small change we do does not seem scalable since we will end up with tons of API versions we will need to keep track of.

@venkatcmsft
Copy link
Copy Markdown

Hi Gaurav - this is how we have shipped SDK for last 3 yrs. We internally talked about this and potentially changing this. We talked to few other teams and decided to continue this model as others are doing the same and also we have not heard any feedback so far on this model. Given the cost of changing API version for every change, we are only changing API version for any breaking changes for existing behaviors/APIs. all additive changes are treated as non-breaking currently.

@ravbhatnagar
Copy link
Copy Markdown
Contributor

@venkatcmsft @atpham256 - I think we discussed this in our call at length and we had agreement that this was the right thing to do for the customers. Otherwise, how will be customers know when you start supporting new capabilities in your service (ex - by adding new properties etc.). This is an Azure wide standard we want to follow and push teams towards. The fact that some other team is also not following the recommendation should not be the benchmark here.
Lets make sure we do the right thing for the customers. If you feel, versioning APIs in such cases is not the right thing to do, then lets have a call to discuss this further.

@venkatcmsft
Copy link
Copy Markdown

Gaurav - Yes, we had a meeting about it. We then talked internally about how to execute on it. We concluded that incrementing API version for non-breaking additive changes is not required. reasons: Unless users do strict schema validation of the response, they would not fail on deserialization and that is not common (we have 15k unique customers and no one ever complained about this as this is how APIs have been developed for the last 3 yrs in this service team). That is the reason we went around talking to various teams about how they are managing this kind of changes and found that it has been a problem for these other services we talked to as well. so, they decided to do what is working for their customers.
I agree with coming up with standard guidelines to enforce so customers have consistent experience. However, the model you are proposing is too strict and results in ugly code to have to manage lots of API version based checks for supporting features in code.

Let us take it offline and close on this as it is hard to get to closure on this thread.

@johanste
Copy link
Copy Markdown
Member

Please note that if you are using AutoRest to generate clients, strong client side validation will indeed be happening (at least in Python and Node/javascript). The client will fail the deserialization when discriminator values that it does not know about are encountered.

@atpham256
Copy link
Copy Markdown
Contributor Author

@ravbhatnagar Hi Gaurav, as discussed on Friday, can we approve this PR as an exception to unblock work for a dependent team? These 2 new polymorphic types have also already been exposed in this API version for a long time so we would like them to be in the Swagger spec to accurately document the API version.

@ravbhatnagar
Copy link
Copy Markdown
Contributor

@olydis - It was agreed over skype call that we will let this through since this change has been in on the service side for a long time now. This has been the model Stream Analytics has been following when adding new properties to existing APIs etc. And so, although not ideal, we are approving this PR. Stream Analytics team will take a workitem on their side to start support api-versioning for their service for such scenarios.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 28, 2017
@salameer
Copy link
Copy Markdown
Member

@olydis, I think the ball is in your court now :)

@atpham256
Copy link
Copy Markdown
Contributor Author

@olydis Ping! :)

@olydis
Copy link
Copy Markdown
Contributor

olydis commented Aug 30, 2017

reviewing 😉

Copy link
Copy Markdown
Contributor

@olydis olydis left a comment

Choose a reason for hiding this comment

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

LGTM

@olydis olydis merged commit 09f3c92 into Azure:current Aug 30, 2017
@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-node

@AutorestCI
Copy link
Copy Markdown

@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-python

mccleanp pushed a commit that referenced this pull request Mar 23, 2022
* create MS.Codespaces RP

* fixing validation errors

* fixing integer validation errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants