Skip to content

Add GetRehydrationtoken for rehydration purpose #41655

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Feb 20, 2024
Merged

Add GetRehydrationtoken for rehydration purpose #41655

merged 20 commits into from
Feb 20, 2024

Conversation

@live1206
Copy link
Member

@live1206 live1206 commented Jan 30, 2024 •
edited
Loading

Instead of reusing the existing Operation.Id, which usually represents the Guild of the operation.
We would like to introduce a method to return the RehydrationToken, which will be used to rehydrate the long-running operation.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core

@live1206 live1206 marked this pull request as ready for review January 30, 2024 09:18
@live1206 live1206 marked this pull request as draft January 30, 2024 09:21
@live1206 live1206 mentioned this pull request Jan 30, 2024
Closed
@live1206 live1206 marked this pull request as ready for review January 30, 2024 15:22
jsquire
jsquire reviewed Jan 30, 2024
View reviewed changes
m-nash
m-nash reviewed Jan 31, 2024
View reviewed changes
live1206
live1206 commented Feb 1, 2024
View reviewed changes
m-nash
m-nash approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@m-nash m-nash left a comment

Choose a reason for hiding this comment

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

LGTM. Lets let @jsquire and @KrzysztofCwalina weigh in as well before merging

@jsquire
Copy link
Member

jsquire commented Feb 1, 2024

LGTM. Lets let @jsquire and @KrzysztofCwalina weigh in as well before merging

I don't have enough context to feel comfortable approving, but my concerns around mocking were addressed. I'd like to see a protected constructor, but it's non-blocking.

KrzysztofCwalina
KrzysztofCwalina reviewed Feb 1, 2024
View reviewed changes
@live1206
Copy link
Member Author

live1206 commented Feb 5, 2024 •
edited
Loading

@KrzysztofCwalina Could you have a final review on this contract? Thanks!
In the meanwhile, I have sent out a note regarding the possibility of shrinking the contract. Basically JS needs 5 fields, not a big gap from our 6 fields contract, but they have a different algorithm to transform the original data.

KrzysztofCwalina
KrzysztofCwalina approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina left a comment

Choose a reason for hiding this comment

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

The APIs look good to me

annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

Please add an entry to the CHANGELOG for this change

annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

Would it be possible to add tests to this PR to validate that the functionality is correct?

annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
annelo-msft
annelo-msft reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

Please add a description to this PR and links to the GH issues/arch board review issues tracking the work.

@live1206
Copy link
Member Author

live1206 commented Feb 6, 2024 •
edited
Loading

Would it be possible to add tests to this PR to validate that the functionality is correct?

It's not enough to test the LRO rehydration functionality with only the contract in place.
The full e2e test will be located in Azure.ResourceManager, like this

@jsquire
Copy link
Member

jsquire commented Feb 6, 2024

Would it be possible to add tests to this PR to validate that the functionality is correct?

It's not enough to test the LRO rehydration functionality with only the contract in place. The full e2e test will be located in Azure.ResourceManager, like this

Please add unit tests. Features are not added to Azure.Core without explicit test coverage.

@live1206
Copy link
Member Author

live1206 commented Feb 7, 2024 •
edited by jsquire
Loading

@jsquire @annelo-msft Sorry that I am not following here, what kind of unit tests do we require for contracts?
Because Operation.cs is an abstract class, the concrete implementation is within the subtypes. In this PR, we are adding a default implementation for GetRehydrationMethod() to null. We don't have > existing unit tests to cover Operation.cs, either.

You're right in that Operation is abstract and has no built-in functionality. RehydrationToken, however, does. The ability to serialize and deserialize the rehydration token is one of the key scenarios for using it. We'll want test coverage on the serialization to ensure that the behavior is correct and remains so over time.

@live1206
Copy link
Member Author

live1206 commented Feb 8, 2024

@jsquire @annelo-msft Sorry that I am not following here, what kind of unit tests do we require for contracts?
Because Operation.cs is an abstract class, the concrete implementation is within the subtypes. In this PR, we are adding a default implementation for GetRehydrationMethod() to null. We don't have > existing unit tests to cover Operation.cs, either.

You're right in that Operation is abstract and has no built-in functionality. RehydrationToken, however, does. The ability to serialize and deserialize the rehydration token is one of the key scenarios for using it. We'll want test coverage on the serialization to ensure that the behavior is correct and remains so over time.

@jsquire @annelo-msft Added unit tests to cover serialization of RehydrationToken, please take another look. Thanks!

@live1206 live1206 requested a review from a team as a code owner February 19, 2024 03:21
@live1206 live1206 merged commit 0c4198b into main Feb 20, 2024
@live1206 live1206 deleted the rehydration-token branch February 20, 2024 02:52
JoshLove-msft added a commit that referenced this pull request Feb 22, 2024
@JoshLove-msft @vishweshbankwar
@scottaddie @azure-sdk @benbp @glorialimicrosoft @DominikMe @nisha-bhatia @jsquire @mjafferi-msft @dargilco @heaths @cilwerner @christothes @AlexanderSher @weshaggard @m-redding @annelo-msft @antkmsft @pallavit @eerhardt @hallipr @rhurey @amnguye @amcasey @richardcho-msft @JimSuplizio @zhangjiale-64 @live1206 @wangbwn @revanthballa1188 @shrauta-ms @Sandido @ArcturusZhang
Update autorest.csharp (#42127)
bfb25d2 
* Add log filtering processor distro (#41665)

* draft

* fix pooling issue

* file header

* clean up

* Add test

* rmv using

* refactor

* log

* rmv unused

* fix test

* changelog

* feedback

* refactor

* fix test

* Update Packages.Data.props

---------

Co-authored-by: Timothy Mothra <[email protected]>

* disable live metrics perf counters (#41878)

* disable live metricsr perf counters

* changelog

* update changelog

* change AspNetCore to use ProjectReference.

* Document using default OS account with WAM (#41877)

* Document using default OS account with WAM

* Exclude samples folder from project, as it was before

* prep LiveMetrics beta2 (#41879)

* Add stress test arm/bicep template prefix to New-TestResources ResourceType set (#41881)

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Increment package version after release of Azure.Monitor.OpenTelemetry.LiveMetrics (#41883)

* Add GA changes for Azure.Communication.Messages  (#41458)

* iteration 1, addressed renaming, reran live test recordings

* rerun with latest GA swagger, and rerun live test recordings

* revert accidental check in of recordmode change

* rename keyCredential to credential

* add SanitizedHeaders for Repeatability

* Generate SDK from TypeSpec, fix convenience layers

* rerun recording

* add [CodeGenModel("AzureCommunicationMessagesClientOptions")]

* use V2024_02_01 as latest

* Update MessageTemplateItem.cs

* revert accidental check in of recordmode change

* rename keyCredential to credential

* attempt 1

* add Kind to MessageTemplateBindings

* rerun tests

* regenerate

* regenerate: Typespec with Polymorphism

* regenerate current api listing from code

* Azure.Communication.Messages: delete all unnecessary custom code

* fix names and model sub namespace

* remove dead comments and usings

* remove more dead comments

* change operation name from sendMessage to send

* add WhatsAppMessageButtonSubType to Azure.Communication.Messages.Models.Channels namespace

* Use GeoPosition to represent Latitude and Longitude in MessageTemplateLocation

* make WhatsAppMessageTemplateBindings internal

* make Latitude and Longitude internal

* update api listing (please always update with Export-API.ps1)

* Revert "make WhatsAppMessageTemplateBindings internal"

This reverts commit 2706f9d.

* rerun recording

* use latestest typespec

* regenerate with latest typespec - rename url to uri, make downloadmedia internal

* regenerate with latest typespec - fixing some typos

* revert accidental deletion of phoneNumber api

* regenerate from latest TypeSpec

* re-run recordings

* regenerate from latest typespec

* re-run recordings

* revert accidental check in of local testing changes

* add more tests

* add more tests

* rerun recordings

* update tests and rerun recording

* fix test name

* Update README.md

* regenerate and fix build pipeline issue

* fix broken links in readme

* move some tests out of live tests into tests file

* rerun recordings

* re-run recording with proper recorded variables

* change destination path for downloadMedia

---------

Co-authored-by: Dominik Messinger <[email protected]>

* prep azure monitor distro for beta (#41882)

* [Azure.Monitor.Query] Add sovereign/government cloud support (#41653)

* Increment package version after release of Azure.Monitor.OpenTelemetry.AspNetCore (#41889)

* [CODEOWNERS] Use Azure.Core team (#41850)

* [.NET] Communication Common | Microsoft Teams App Identifier feature GA release updates (#41902)

* Azure.Communication.Common release updates

* [Event Hubs] Release Prep: Feb 2024 (#41904)

The focus of these changes is to prepare the Event
Hubs core package for the February 2024 release.

* Avoid race condition when making checkpointing decision (#41891)

* Avoid race condition when making checkpointing decision

* change log

* Use linkedCts when calling TryExecute

* Add feb system events (#41896)

* Add feb system events

* Move Avs event to correct namespace

* API

* Fix typo in Azure.Core ClientOptions.cs comments. Fix typo and use `Entra ID` term in Azure.AI.OpenAI readme. (#41908)

- Fix typo in comments for class sdk/core/Azure.Core/src/ClientOptions.cs
- Fix missing dot in sdk/openai/Azure.AI.OpenAI/README.md
- Use the term Entra ID instead of Azure Active Directory in sdk/openai/Azure.AI.OpenAI/README.md

* Update System.IdentityModel.Tokens.Jwt (#41887)

* Update allowed MHSM locations

* Update System.IdentityModel.Tokens.Jwt

Fixes CVE-2024-21319

* Update Key Vault for API version 7.5 (#41890)

* Update Key Vault for API version 7.5

* Update public APIs

* Update package versions

* Change channel id of GetTemplates operation to GUID (#41909)

* change channel id of getTemplate operation to uuid

* rerun Export-API.ps1

* update version

* Increment package version after release of Azure.Communication.Common (#41924)

* Post-release updates for Azure.Communication.Common (#41927)

* Rebranding Microsoft Entra (#41900)

* Update troubleshooting doc for AADSTS65002 errors (#41906)

* Prepare for release (#41915)

* Use common environment for releasing (#41912)

- Switch to using common package-publish environment for release
- Switch to using checkout instead of manual clone for azure-sdk-build-tools

* [Event Hubs] Fix backwards compatibility with new vs old SDK checkpointing (#41911)

* initial changes

* fix

* update docs/tests

* fix tests

* update docs/logging

* change replication segment to string

* remove generic from WriteEvent methods

* Pass linked token to trigger execution (#41921)

* ClientModel: Move buffering into the transport (#41772)

* Move buffering into the transport

* update

* nit

* pr fb

* move network timeout value initialization to transport

* Update contract for Response.Content

* pr fb

* Add exception if stream position is not 0

* bug fix

* pr fb

* Add CreateAsync factory method to ClientResultException

* Migrate Session Recordings to the Assets Repo (#41939)

* [Event Hubs] Processor Release Prep: Feb 2024 (#41905)

* [Event Hubs] Processor Release Prep: Feb 2024

The focus of these changes is to prepare the Event
Hubs Processor and Functions extension packages for
the February, 2024 release.

* [Event Hubs] Resolve reference issue for release (#41951)

* Handle error when pwsh does not exist language agnostic (#41910)

* Increment package version after release of Azure.Messaging.EventHubs (#41942)

* Fix for drain when prefetch is enabled (#41920)

* Fix for drain when prefetch is enabled

* Fix to prefetch drain

* Add comment

* Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs

Co-authored-by: Jesse Squire <[email protected]>

---------

Co-authored-by: Jesse Squire <[email protected]>

* Fix case-sensitive spelling in scripts that get deployed to C++ repo (#41948)

Co-authored-by: Anton Kolesnyk <[email protected]>

* Increment package version after release of Azure.Messaging.EventGrid (#41943)

* Increment version for extensions releases (#41935)

* Increment package version after release of Azure.Extensions.AspNetCore.DataProtection.Blobs

* Increment package version after release of Microsoft.Extensions.Azure

* Increment package version after release of Azure.Extensions.AspNetCore.DataProtection.Keys

* Increment package version after release of Azure.Extensions.AspNetCore.Configuration.Secrets

* Increment package version after release of Microsoft.Azure.WebJobs.Extensions.EventHubs (#41965)

* Increment package version after release of Azure.Messaging.EventHubs.Processor (#41964)

* Prepare for release (#41970)

* Update the dependencies to resolve vulnerabilities. (#41798)

* Update all dependencies

* Fix the order of the libraries in the packages.data.props

* [Event Hubs] Post-release fixes (#41966)

The focus of these changes is to bump the version
of the processor package used by samples and to
restore the project references for the stress tests.

* [Extensions] Add AOT compatibility attributes to Microsoft.Extensions.Azure (#41936)

* [Extensions] Add AOT compatibility attributes

Annotate the Microsoft.Extensions.Azure library for trimming/AOT compatibility.

Also add a few suppressions to Azure.Identity's EventSource that showed up when I was using Microsoft.Extensions.Azure in a real-world app.

* Add expected AOT warnings for DiagnosticSource.Write

These warnings went away with dotnet/runtime#76109, which was backported to .NET 7.0.3, but the CI build machines are still using an older 7.0.x, which don't have these warnings fixed. Adding expected warnings entries for them until we take a new .NET version in CI.

* Fix CI errors.

* Add CheckAOTCompat to Azure.Identity

* Fix up Identity expected aot warnings for CI.

* Move emitter-package.json scripts to eng/common (#41989)

Co-authored-by: Patrick Hallisey <[email protected]>

* Beta 2 updates (#41913)

* Beta 2 updates

* API update

* Changelog updates

* [Analyzers] Adding additional approved namespaces (#41967)

* [Analyzers] Adding additional approved namespaces

The focus of these changes is to update the approved namespaces to include:
- Azure.Compute
- Azure.Health
- Azure.Verticals

* Move to new package version

* ClientModel: Make ClientRetryPolicy.Default a property instead of a field (#41839)

* Make ClientRetryPolicy.Default a static property instead of a field

* CHANGELOG

* pr fb

* Fix spelling mistakes (#41998)

* Throw RequestFailedException for failed backup/restore (#41919)

* Throw RequestFailedException for failed backup/restore

Fixes #41855

* Update release dates

* Increment version for keyvault releases (#42003)

* Increment package version after release of Azure.Security.KeyVault.Secrets

* Increment package version after release of Azure.Security.KeyVault.Certificates

* Increment package version after release of Azure.Security.KeyVault.Administration

* Increment package version after release of Azure.Security.KeyVault.Keys

* Increment package version after release of Azure.AI.Vision.ImageAnalysis (#42002)

* Increment package version after release of Azure.Messaging.ServiceBus (#41973)

* Fix for bug on Rename where a SAS appended to the Uri, does not get passed to the destination which does not have credentials (#42004)

* Correct unit mismatch in backoff calculation (#42011)

While debugging locally, I noticed some unusually long delays and tracked it back to the fact that delays are computed in ticks but applied in milliseconds.

* Add Call Failed events (#41950)

* update swagger ref

* fix typo

* update swagger

* generate file

* add to eventParser

* add eventParser tests

* fix mute tests

* fix tests

* update eventprocessor

* update api change

* Update GitHubEventProcessorVersion (#42031)

Co-authored-by: James Suplizio <[email protected]>

* ClientModel: PipelineRequest and PipelineResponse API updates (#42010)

* change Core methods to Core properties

* rename ReadContent->BufferContent and export APIs

* nits

* Make PipelineRequest.Uri nullable

* Updates to tools for the CodeownersUtils updates (#42032)

Co-authored-by: James Suplizio <[email protected]>

* Update Generator Version 3.0.0-beta.20240216.2 (#42039)

* ClientModel: Get test coverage parity with Azure.Core (#42020)

* add tests for response.Content

* add tests and remove unused code

* Add test coverage

* PR fb

* fix build break (#42046)

* Update AutoRest C# version to 3.0.0-beta.20240217.1 (#42055)

* Fix Communication CallAutomation README Issue (#41741)

* Add GetRehydrationtoken for rehydration purpose (#41655)

* Test avro and json dataset schema. (#41699)

* Update Generator Version 3.0.0-beta.20240219.2 (#42074)

* Add E5 to SDK (#41898)

* Increment package version after release of Azure.ResourceManager.RedisEnterprise (#42078)

* ASR August release version. (#41848)

* Update AutoRest C# version to 3.0.0-beta.20240220.3 (#42099)

* Update Generator Version 3.0.0-beta.20240220.3

* Update SDK codes de_he_2

* Update SDK codes hy_mi_3

* Update SDK codes ad_co_0

* Update SDK codes sq_wo_6

* Update SDK codes mi_pu_4

* Update SDK codes co_da_1

* Update SDK codes pu_sq_5

* Update AutoRest C# version to 3.0.0-beta.20240221.1 (#42104)

* Update Generator Version 3.0.0-beta.20240221.1

* Update SDK codes de_he_2

* Update SDK codes hy_mi_3

* Update SDK codes ad_co_0

* Update SDK codes sq_wo_6

* Update SDK codes mi_pu_4

* Update SDK codes co_da_1

* Update SDK codes pu_sq_5

* Update autorest version

* regen

* GalleryRP 2023-07-03 release 2nd (#41994)

Co-authored-by: ArcturusZhang <[email protected]>

* regen

* Update AutoRest C# version to 3.0.0-beta.20240221.2 (#42125)

---------

Co-authored-by: Vishwesh Bankwar <[email protected]>
Co-authored-by: Timothy Mothra <[email protected]>
Co-authored-by: Scott Addie <[email protected]>
Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Ben Broderick Phillips <[email protected]>
Co-authored-by: glorialimicrosoft <[email protected]>
Co-authored-by: Dominik Messinger <[email protected]>
Co-authored-by: nisha-bhatia <[email protected]>
Co-authored-by: Jesse Squire <[email protected]>
Co-authored-by: Maqsood Jafferi <[email protected]>
Co-authored-by: Darren Cohen <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
Co-authored-by: Chris Werner <[email protected]>
Co-authored-by: Christopher Scott <[email protected]>
Co-authored-by: Alexander Sher <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Madalyn Redding <[email protected]>
Co-authored-by: Anne Thompson <[email protected]>
Co-authored-by: Jesse Squire <[email protected]>
Co-authored-by: Anton Kolesnyk <[email protected]>
Co-authored-by: Pallavi Taneja <[email protected]>
Co-authored-by: Eric Erhardt <[email protected]>
Co-authored-by: Patrick Hallisey <[email protected]>
Co-authored-by: Ryan Hurey <[email protected]>
Co-authored-by: Amanda Nguyen <[email protected]>
Co-authored-by: Andrew Casey <[email protected]>
Co-authored-by: richardcho-msft <[email protected]>
Co-authored-by: James Suplizio <[email protected]>
Co-authored-by: Jiale Zhang (MSFT) <[email protected]>
Co-authored-by: Wei Hu <[email protected]>
Co-authored-by: wangbwn <[email protected]>
Co-authored-by: revanthballa1188 <[email protected]>
Co-authored-by: shrauta-ms <[email protected]>
Co-authored-by: Adam Sandor <[email protected]>
Co-authored-by: ArcturusZhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scottaddie scottaddie scottaddie left review comments

@annelo-msft annelo-msft annelo-msft left review comments

@jsquire jsquire jsquire approved these changes

@KrzysztofCwalina KrzysztofCwalina KrzysztofCwalina approved these changes

@m-nash m-nash m-nash approved these changes

@tg-msft tg-msft Awaiting requested review from tg-msft

@JoshLove-msft JoshLove-msft Awaiting requested review from JoshLove-msft

@christothes christothes Awaiting requested review from christothes

Assignees

No one assigned

Labels

Azure.Core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@live1206 @azure-sdk @jsquire @KrzysztofCwalina @scottaddie @annelo-msft @m-nash