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

simplify context and move set offset to context commit #34

Merged
merged 3 commits into from
Sep 12, 2017

Conversation

db7
Copy link
Collaborator

@db7 db7 commented Sep 11, 2017

No description provided.

@@ -213,6 +214,7 @@ func (ctx *context) setValueForKey(key string, value interface{}) error {
return fmt.Errorf("Cannot set nil as value.")
}

ctx.counters.stores++
Copy link
Contributor

@heltonmarx heltonmarx Sep 11, 2017

Choose a reason for hiding this comment

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

You're increase this variable here, but probably is reading in other coprocess. For this case, it's necessary protect it with mutex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it cannot/should not be accessed concurrently.

  • counters.stores can only be incremented by the goroutine that creates the context, which is the partition-goroutine.
  • promise-goroutines (coming from emit confirmations) or the partition-goroutine may read stores if they call ctx.commit()
  • but only one of them will call it (the last one calling tryCommit()) and that happens after at least one lock-protected critical section, which will act as a memory barrier anyways.

The comment made me realize though, that counters.calls doesn't need a lock too. So I removed that lock and also renamed the counters.

m sync.Mutex
wg *sync.WaitGroup
errors Errors
m sync.Mutex
Copy link
Contributor

@heltonmarx heltonmarx Sep 11, 2017

Choose a reason for hiding this comment

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

This mutex can be substituted by a RWMutex, considering that you're need create a method like:

func (ctx *context) Stores() int {
	ctx.m.RLock()
	defer ctx.m.RUnlock()
	return ctx.counters.stores
}

@db7 db7 closed this Sep 12, 2017
@db7 db7 deleted the reorder-offset-store branch September 12, 2017 08:24
@db7 db7 restored the reorder-offset-store branch September 12, 2017 08:24
@db7 db7 reopened this Sep 12, 2017
@db7 db7 merged commit 78edea6 into master Sep 12, 2017
@db7 db7 deleted the reorder-offset-store branch September 12, 2017 08:26
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