Skip to content

Refactor move servicebus message state key to azure-core-amqp#29309

Merged
ZejiaJiang merged 5 commits intoAzure:mainfrom
ZejiaJiang:refactor/move-msg-state
Jun 30, 2022
Merged

Refactor move servicebus message state key to azure-core-amqp#29309
ZejiaJiang merged 5 commits intoAzure:mainfrom
ZejiaJiang:refactor/move-msg-state

Conversation

@ZejiaJiang
Copy link
Member

@ZejiaJiang ZejiaJiang commented Jun 8, 2022

Description

Refactor: move servicebus message state key to azure-core-amqp constants. #26898

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@ghost ghost added Azure.Core azure-core Service Bus labels Jun 8, 2022
@azure-sdk
Copy link
Collaborator

azure-sdk commented Jun 8, 2022

API change check

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

azure-core-amqp

@ZejiaJiang ZejiaJiang changed the title Refactor move servicebus message state key to amqp core Refactor move servicebus message state key to azure-core-amqp Jun 9, 2022
@ZejiaJiang ZejiaJiang marked this pull request as ready for review June 13, 2022 07:45
@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Jun 13, 2022

you might need to wait after servicebus June release (amqp released, but servicebus is not yet).

@weidongxu-microsoft
Copy link
Member

I think servicebus is released, you might want to refresh main and get it merged before next code freeze.

/**
* The state of message.
*/
SERVICE_BUS_MESSAGE_STATE_ANNOTATION_NAME("x-opt-message-state");
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for why we change the name from SERVICE_BUS_MESSAGE_STATE_KEY to SERVICE_BUS_MESSAGE_STATE_ANNOTATION_NAME?

Since we move it into amqp, is it ok to remove SERVICE_BUS prefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the above keys have ANNOTATION_NAME suffix as well and we do get the key from annotation.

I'll check these keys usage and remove the prefix.

Copy link
Contributor

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

The creation and use of the unreleased_ tag was done correctly and I'm okay with those changes. I'll let the other people review and approve the corresponding code changes.

@ZejiaJiang ZejiaJiang merged commit 04ac4f9 into Azure:main Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[REFACTOR] Move x-opt-message-state from package-private constant into azure-core-amqp

7 participants