-
Notifications
You must be signed in to change notification settings - Fork 14
feat(item): enhance update item to handle ItemDisplayNameNotAvailableYet when display name is reserved for 5 min #501
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR enhances the update item functionality by introducing a retry mechanism to handle cases where an item's display name is reserved (returning the "ItemDisplayNameNotAvailableYet" error). Key changes include replacing direct calls to UpdateItem with RetryUpdateItem across several resource files and adding new retry functions and configurations in helpers.go.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
internal/pkg/fabricitem/resource_item_properties.go | Replaces direct UpdateItem call with RetryUpdateItem |
internal/pkg/fabricitem/resource_item_definition_properties.go | Replaces direct UpdateItem call with RetryUpdateItem |
internal/pkg/fabricitem/resource_item_definition.go | Replaces direct UpdateItem call with RetryUpdateItem |
internal/pkg/fabricitem/resource_item_config_properties.go | Replaces direct UpdateItem call with RetryUpdateItem |
internal/pkg/fabricitem/resource_item_config_definition_properties.go | Replaces direct UpdateItem call with RetryUpdateItem |
internal/pkg/fabricitem/resource_item.go | Replaces direct UpdateItem call with RetryUpdateItem |
internal/pkg/fabricitem/helpers.go | Adds retry logic functions and configuration for handling update errors |
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.
add tests
Minimum allowed line rate is |
var respUpdate fabcore.ItemsClientUpdateItemResponse | ||
var err error | ||
|
||
err = RetryOperation(ctx, DefaultUpdateRetryConfig(), func() error { |
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.
IMO updating inside of the retry operation is a bit odd,
why not pulling the update to be done before the retry function call, and enter it only if there is an error
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.
Inside the retry function I'd need to call update again, so that's duplicate code.
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.
alternatively, I can call the outer function "CreateItem" or "UpdateItem" and the internal implementation can reflect the retry mechanism, WDYT?
…r-fabric into dev/badeamarjieh/handle_name_temporarily_unavailable
📥 Pull Request
❓ What are you trying to address
ItemDisplayNameNotAvailableYet