-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix data distribution tracker memory bugs #4076
Fix data distribution tracker memory bugs #4076
Conversation
bf54328
to
9014720
Compare
9014720
to
ca8ea3b
Compare
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.
I like this general approach
state DataDistributionTracker self(cx, distributorId, readyToStart, output, shardsAffectedByTeamFailure, | ||
anyZeroHealthyTeams, *shards); | ||
anyZeroHealthyTeams, *shards, *trackerCancelled); |
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.
What if we pass a copy of GetTracker
to child actors here instead of a pointer to self
? I think that would make it harder to accidentally dereference self
after it's freed
// Ideally this would be implemented with a lambda instead, but the actor compiler does not do | ||
// type deduction. | ||
class GetTracker { | ||
bool const& trackerCancelled; |
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.
This is easier to reason about for me if the members are pointers /shrug
@@ -658,8 +658,7 @@ ACTOR Future<Void> shardTracker(DataDistributionTracker::SafeAccessor self, KeyR | |||
} | |||
} catch (Error& e) { | |||
// If e is broken_promise then self may have already been deleted |
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.
This comment is stale too now
Changes in this PR:
Style
git clang-format
).Performance
std::vector
vsVectorRef
).SlowTask
traces.Testing
ASSERT
,ASSERT_WE_THINK
, andTEST
macros are added in appropriate places.