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

Remove namespace field from ScheduleActivityTaskCommandAttributes message #2753

Merged

Conversation

alexshtin
Copy link
Member

What changed?
Remove namespace field from ScheduleActivityTaskCommandAttributes message.

Why?
To remove ability of cross namespace activity calls.
See temporalio/api#174 for more details.

How did you test it?
Existing tests.

Potential risks
Low risk: namespace field is not exposed for the most SDKs but there might be some unofficial SDK which will loose ability to start activity in another namespace.

Is hotfix candidate?

@alexshtin alexshtin requested a review from a team as a code owner April 21, 2022 00:27
@alexshtin alexshtin force-pushed the feature/remove-cross-ns-activity branch from e41619d to 3766979 Compare April 21, 2022 00:29
Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

Do you plan to update history activity transfer task definition and matching AddActivityTask request as well?

Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

Will there be any backward/forward compatibility issues when rolling out the change?

@alexshtin alexshtin force-pushed the feature/remove-cross-ns-activity branch from 9e76cf4 to da8a74d Compare April 22, 2022 21:22
@alexshtin alexshtin force-pushed the feature/remove-cross-ns-activity branch from c837627 to 37855be Compare April 23, 2022 00:46
@alexshtin
Copy link
Member Author

Will there be any backward/forward compatibility issues when rolling out the change?

Good point. Now it does!

@yycptt
Copy link
Member

yycptt commented Apr 23, 2022

Some more context for what we are trying to do here.
In the next release (1.17), we will continue to write to:

  • TargetNamespaceId in persistencespb.TransferTaskInfo,
  • ActivityInfo.namespace_id proto field,
  • AddActivityTaskRequest.source_namespace_id proto field.

But no logic will use those three fields. And in 1.18 those write logic will also be removed.

@alexshtin alexshtin enabled auto-merge (squash) April 23, 2022 02:29
@alexshtin alexshtin merged commit 58103d7 into temporalio:master Apr 23, 2022
@alexshtin alexshtin deleted the feature/remove-cross-ns-activity branch April 23, 2022 05:57
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.

None yet

2 participants