Skip to content

Time Series Insights Instances client library implementation#19209

Merged
barustum merged 22 commits intomasterfrom
feature/timeseriesinsights/barustum/instancesImplementation
Mar 10, 2021
Merged

Time Series Insights Instances client library implementation#19209
barustum merged 22 commits intomasterfrom
feature/timeseriesinsights/barustum/instancesImplementation

Conversation

@barustum
Copy link
Copy Markdown
Contributor

@barustum barustum commented Mar 4, 2021

Here are some things to keep in mind before/while you are reviewing this PR:

  • Swagger and examples: https://github.com/Azure/azure-rest-api-specs/tree/master/specification/timeseriesinsights/data-plane/Microsoft.TimeSeriesInsights/stable/2020-07-31
  • I would start with reviewing TimeSeriesId classes. Client library implementation references TimeSeriesId classes often, so it'll be useful to know these before other things. If there are unclear pieces of code that you comment on, it means I lack documentation in explaining what a piece of code does. So please do let me know if there is anything unclear
  • If you need a refresher on what Time Series Id is, or any TSI related topic, please reach out to me or Binal
  • The design of TimeSeriesId classes are yet to be finalized as we are discussing with Krzysztof alternative designs
  • This PR does not include the implementation of the Search API. I will do Search in a separate change
  • Client libraries are like jokes. If you have to explain them, it isn't that good :). So let me know if things are unclear and we can work on making them clearer.

Copy link
Copy Markdown
Contributor

@bikamani bikamani left a comment

Choose a reason for hiding this comment

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

Added few comments, looks good otherwise

/// <exception cref="ArgumentException">
/// The exception is thrown when <paramref name="timeSeriesInstances"/> is empty.
/// </exception>
public virtual Response<InstancesOperationError[]> CreateOrReplaceTimeSeriesInstances(
Copy link
Copy Markdown
Contributor

@bikamani bikamani Mar 9, 2021

Choose a reason for hiding this comment

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

Something I just noticed now, did you intend to create a separate API for "replace" or combined for "create and replace"?

Looking at code at line: 772

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually looking at swagger, I realize there is put (createOrreplace and update (just replace) - which is super confusing.

@barustum barustum merged commit 596091f into master Mar 10, 2021
@barustum barustum deleted the feature/timeseriesinsights/barustum/instancesImplementation branch March 10, 2021 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants