-
Notifications
You must be signed in to change notification settings - Fork 14
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
Branch Scheduling #226
base: main
Are you sure you want to change the base?
Branch Scheduling #226
Conversation
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 think this is close to a good sequencing point, aside from the specific comments I added.
Also, if you want, I can go in and remove the iface field from MPCalContext myself. It staying in place makes me nervous.
distsys/archetypeinterface.go
Outdated
iface.ForkedResources[forkedHandle] = forkedResource | ||
} else { | ||
// Parent iface had the resource so link its state | ||
err := forkedResource.LinkState() |
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.
imo, this method should not exist. I think there's another way, where you just manipulate the maps and nothing else, until commit.
distsys/archetypeinterface.go
Outdated
iface.ForkedResources[forkedHandle] = forkedResource | ||
} else { | ||
// Parent iface had the resource so abort its state | ||
err := forkedResource.AbortState() |
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.
similar theme as LinkState, why isn't this just Abort?
distsys/archetyperesource.go
Outdated
} | ||
|
||
func (res *LocalArchetypeResource) LinkState() error { | ||
res.forkParent.value = res.value |
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.
following on for why this method should not exist imo, notice how there would be literally no difference if you just kept one of the forked resources, vs doing a field by field full copy here.
…to ruchitp-branch-scheduling
…g "complicated" example in incmap.go. Warning: this will not compile!
…to ruchitp-branch-scheduling
// ForkState provides a copy to the ArchetypeResourceState such that operations performed on other ArchetypeResourceStates | ||
// with the same parent ArchetypeResource will not cause side effects for the current ArchetypeResourceState. | ||
// ForkState will clone all the parts of a resource whose properties are NOT idempotent. A call to ForkState | ||
// creates the expectation that there will be an eventual call to Commit/Abort on the resulting ArchetypeResourceState |
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.
Suggested rephrasing:
ForkState will clone all the parts of a resource whose properties are NOT idempotent.
ForkState should create a separate copy of any thread-local data held by the original ArcvhetypeResourceState. It is the responsibility of the implementation to ensure side-effects that are not thread-local are synchronized and maintained correctly, given that the two forked versions will be accessed by different goroutines.
@fhackett should we close? |
No description provided.