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

fix: solve data race #234

Merged
merged 5 commits into from
Jul 28, 2023
Merged

fix: solve data race #234

merged 5 commits into from
Jul 28, 2023

Conversation

Jeremy-Run
Copy link
Contributor

  • solve data race
  • modify annotation

@Jeremy-Run Jeremy-Run changed the title solve data race fix:solve data race Jul 28, 2023
@Jeremy-Run Jeremy-Run changed the title fix:solve data race FIX:solve data race Jul 28, 2023
@roseduan
Copy link
Collaborator

Thanks, then go test -race will pass, right?

@Jeremy-Run
Copy link
Contributor Author

yes

@Jeremy-Run Jeremy-Run changed the title FIX:solve data race fix: solve data race Jul 28, 2023
}()
}
wg.Wait()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some assertions after the concurrent put is done.
like Assert.equal(100, KeysNum)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

batch.go Outdated
@@ -16,7 +16,7 @@ import (
// If readonly is false, you can use Put and Delete method to write data to the batch.
// The data will be written to the database when you call Commit method.
//
// Batch is not a transaction, it does not guarantee isolation.
// Batch is not a transaction, it does not guarantee isolation(only guarantee read committed).
Copy link
Collaborator

Choose a reason for hiding this comment

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

It`s not a very accurate description, actually, the isolation is Serializable because we have a global lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this problem just now, I will delete it.

@@ -174,8 +174,8 @@ func TestDB_Merge_6_Appending(t *testing.T) {
for i := 0; i < 10000; i++ {
key := utils.GetTestKey(rand.Int())
m.Store(string(key), struct{}{})
err = db.Put(key, utils.RandomValue(128))
assert.Nil(t, err)
e := db.Put(key, utils.RandomValue(128))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

@Jeremy-Run Jeremy-Run Jul 28, 2023

Choose a reason for hiding this comment

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

Yes, it's necessary. There are two data race points, this is one.

@roseduan
Copy link
Collaborator

Thanks

@roseduan roseduan merged commit 1e32433 into rosedblabs:main Jul 28, 2023
2 checks passed
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.

2 participants