Skip to content
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

Rename namespace persistence API #2595

Merged
merged 4 commits into from
Mar 10, 2022

Conversation

alexshtin
Copy link
Member

What changed?
Rename namespace persistence API.

Why?
To be able to rename namespaces internally.

How did you test it?
New unit test.

Potential risks
No risks.

Is hotfix candidate?
No.

@alexshtin alexshtin requested a review from a team as a code owner March 9, 2022 18:35
@alexshtin alexshtin marked this pull request as draft March 9, 2022 18:35

if !applied {
// TODO: Update namepspaces_by_id to previous name before returning error???
return serviceerror.NewUnavailable(fmt.Sprintf("RenameNamespace operation failed because of conditional failure."))
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be an CAS related error?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, notification_version in metadata row is also update there.


// Does almost everything in one batch. The only separate thing is namespaecs_by_id update which is almost not used now.
// Can be retried but data between steps is inconsistens (points to the name which doesn't exist).
func (m *MetadataStore) RenameNamespace1(request *p.InternalRenameNamespaceRequest) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer option 1

the additional namespace ID -> namespace name mapping is to ensure namespace ID being unique
the ID -> name mapping (DB table) is not used according to my memory

the ID -> name lookup is done by namespace cache

  • loading all namespace from namespaces table
  • index ID -> name using above records

Copy link
Contributor

Choose a reason for hiding this comment

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

could you add some comment about the namespaces_by_id table? thanks
btw, to successfully achieve the namespace renaming functionality, we need to guarantee only IDs are used within service boundary

Copy link
Member Author

Choose a reason for hiding this comment

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

I improved comment here. I can't expose RenameNamespace as public API for now. I added it only to support namespace deletion.

@alexshtin alexshtin marked this pull request as ready for review March 9, 2022 23:59
@alexshtin alexshtin merged commit f994ea5 into temporalio:master Mar 10, 2022
@alexshtin alexshtin deleted the feature/rename-namespace branch March 10, 2022 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants