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

Refactoring: rename UpdateActivityWithCallback method on mutable state to UpdateActivity #6832

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

ychebotarev
Copy link
Contributor

@ychebotarev ychebotarev commented Nov 16, 2024

What changed?

  1. I change every UpdateActivity to UpdateActivityWithCallback everywhere in the code.
  2. I removed old UpdateActivity
  3. I rename UpdateActivityWithCallback to UpdateActivity
  4. Fix tests, etc.

Why?

Old UpdateActivity was not actually working. ActivityInfo in pending activities is a pointer, so calculating size was wrong. Few other reasons.

How did you test it?

unit tests

Risks

With this change the size of mutable state is calculated properly. This means that in some cases it may change, thus becoming larger then before, and trigger some errors.

Documentation

Is hotfix candidate?

No

@ychebotarev ychebotarev requested a review from a team as a code owner November 16, 2024 00:05
@alexshtin alexshtin changed the title Refactoring - UpdateActivity Refactoring: rename UpdateActivityWithCallback method on mutable state to UpdateActivity Nov 16, 2024
Copy link
Member

@alexshtin alexshtin left a comment

Choose a reason for hiding this comment

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

Can you please also add risk section to PR description saying that some mutable states might start to trigger errors now because their size is computed correctly and will become large than previous incorrect value.

@@ -389,12 +389,13 @@ func (r *TaskRefresherImpl) refreshTasksForActivity(
}

if CompareVersionedTransition(minVersionedTransition, EmptyVersionedTransition) == 0 { // Full refresh
// clear activity timer task mask for later activity timer task re-generation
activityInfo.TimerTaskStatus = TimerTaskStatusNone

// need to update activity timer task mask for which task is generated
if err := mutableState.UpdateActivity(
Copy link
Member

Choose a reason for hiding this comment

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

cc @xwduan is adding a dedicated method for updating timer task status since mutable state needs to know if the updated field supposed to be replicated.

@ychebotarev ychebotarev merged commit 3a1b707 into main Nov 18, 2024
49 checks passed
@ychebotarev ychebotarev deleted the y_refactor branch November 18, 2024 22:10
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.

4 participants