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

Branch Scheduling #226

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Branch Scheduling #226

wants to merge 18 commits into from

Conversation

ruchitpalrecha
Copy link

No description provided.

Copy link
Contributor

@fhackett fhackett left a 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.

iface.ForkedResources[forkedHandle] = forkedResource
} else {
// Parent iface had the resource so link its state
err := forkedResource.LinkState()
Copy link
Contributor

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.

iface.ForkedResources[forkedHandle] = forkedResource
} else {
// Parent iface had the resource so abort its state
err := forkedResource.AbortState()
Copy link
Contributor

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?

}

func (res *LocalArchetypeResource) LinkState() error {
res.forkParent.value = res.value
Copy link
Contributor

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.

// 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
Copy link
Contributor

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.

@shayanh
Copy link
Contributor

shayanh commented Nov 17, 2022

@fhackett should we close?

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

3 participants