[ServiceBus] Abandon not needed if AutoComplete is false #23423
Conversation
ghost
added
Service Bus
customer-reported
|
Thank you for your contribution @zamzowd! We will review the pull request and get back to you soon. |
ghost
added
the
Community Contribution
|
I'm not sure how I can "complete legal signoff" or get this code "fully code reviewed" as described in the Code Review Process |
jsquire
added
the
Client
jsquire
requested review from
DorothySun216,
yvgopal,
vinaysurya,
bainian12345,
shankarsama and
aresowj
|
Hi @zamzowd. Thank you for your contribution and for helping to improve the Azure SDK experience. I've added some tags and routed to the team member best able to offer feedback. |
|
Thanks for the assistance! I noticed this morning that there's also Comparing the documentation, the wording for what AutoComplete does is different in
Ultimately, I'd like to be able to manually deadletter and then throw (for logging purposes). With AutoComplete off, I would expect what the ServiceBusTrigger documentation to describe: it also does not "AutoAbandon" when an exception is thrown. Finally, and to reiterate, I'm not sure about my approach to unit testing this. I couldn't work out a way to unit test with any of the existing |
|
@JoshLove-msft: Would you be so kind as to offer your thoughts on the |
sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ReceiverManager.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusProcessorOptions.cs
Outdated
Show resolved
Hide resolved
|
I'd suggest splitting out the Microsoft.Azure.ServiceBus changes into a separate PR from the Azure.Messaging.ServiceBus changes. I'm ready to approve the changes for Azure.Messaging.ServiceBus, but the changes for Microsoft.Azure.ServiceBus would need to be reviewed by the other reviewers on this PR. |
|
@JoshLove-msft |
zamzowd
force-pushed
the
zamzowd/abandon-autocomplete-only
branch
from
eae8979 to
ea3e8d0
Compare
|
I rebased after the other PR was completed, as mentioned above. This PR no longer has anything in Azure.Messaging.ServiceBus, only in Microsoft.Azure.ServiceBus's MessageReceivePump and SessionReceivePump. |
|
I'm not sure if there's anything further needed before exiting Draft on this PR, so I went ahead with Ready for Review. As I understand it, primarily hoping for review from @DorothySun216 or @shankarsama as the code owners? |
ghost
added
the
no-recent-activity
|
Hi @zamzowd. Thank you, for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
ghost
closed this
|
Hi @zamzowd. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass. |
{AzurePostgresql} PostgreSQL flexibleServers doesn't support creation of 'SystemAssigned' resource identity (Azure#23423)
Fixes Azure/azure-rest-api-specs#23176
Per the [latest API](https://github.com/Azure/azure-rest-api-specs/edit/main/specification/postgresql/resource-manager/Microsoft.DBforPostgreSQL/stable/2022-12-01/FlexibleServers.json), the flexibleServers should support creation of 'SystemAssigned' , but it doesn't . See the below for details:
Request:
PUT /subscriptions/?/resourceGroups/acctestRG-postgresql-230320155838217513/providers/Microsoft.DBforPostgreSQL/flexibleServers/acctest-fs-230320155838217513?api-version=2022-12-01
```
{"identity":{"type":"SystemAssigned"},"location":"eastus","properties":{"administratorLogin":"adminTerraform","administratorLoginPassword":"QAZwsx123","availabilityZone":"2","backup":{"geoRedundantBackup":"Disabled"},"highAvailability":{"mode":"Disabled"},"network":{},"storage":{"storageSizeGB":32},"version":"12"},"sku":{"name":"Standard_D2s_v3","tier":"GeneralPurpose"},"tags":{}}
```
Response:
```
{"error":{"code":"CannotSetResourceIdentity","message":"Resource type 'Microsoft.DBforPostgreSQL/flexibleServers' does not support creation of 'SystemAssigned' resource identity. The supported types are 'UserAssigned'."}}
```
Resolving issues Azure/azure-functions-servicebus-extension#183 and Azure/azure-functions-servicebus-extension#142
If
AutoCompleteisfalse, then it should not attempt to abandon the message.This change brings
AbandonMessageIfNeededAsyncin line withCompleteMessageIfNeededAsync.I'm not sure if this counts as a breaking change. It's fixing an issue to bring it in line with the documentation, but that results in different behavior - throwing an exception will no longer cause the message to be abandoned if AutoComplete is false.
There were no existing unit tests for
MessageReceivePumpso I attempted to add some. Guidance welcome.All SDK Contribution checklist:
This checklist is used to make sure that common guidelines for a pull request are followed.
Draftmode if it is:General Guidelines and Best Practices
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK. Please double check nuget.org current release version.Additional management plane SDK specific contribution checklist:
Note: Only applies to
Microsoft.Azure.Management.[RP]orAzure.ResourceManager.[RP]Management plane SDK Troubleshooting
If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add
new servicelabel and/or contact assigned reviewer.If the check fails at the
Verify Code Generationstep, please ensure:generate.ps1/cmdto generate this PR instead of callingautorestdirectly.Please pay attention to the @microsoft.csharp version output after running
generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.Note: We have recently updated the PSH module called by
generate.ps1to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:Old outstanding PR cleanup
Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.