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

Item::ValueCopy #316

Closed
wants to merge 5 commits into from
Closed

Item::ValueCopy #316

wants to merge 5 commits into from

Conversation

manishrjain
Copy link
Contributor

@manishrjain manishrjain commented Nov 11, 2017

Add a ValueCopy function to avoid deadlocks in long running iterations.

Helps solve #315.


This change is Reviewable

Add a ValueCopy function to avoid deadlocks in long running iterations.
@peterstace
Copy link

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.


iterator.go, line 89 at r1 (raw file):

}

// ValueCopy returns a copy of the value of the item from the value log.

Would be good to indicate in the comment how dst is used. A user would have to read the code to know that dst will be written to starting at dst[0] rather than being appended to.


Comments from Reviewable

@peterstace
Copy link

:lgtm:


Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@deepakjois
Copy link
Contributor

:lgtm:


Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


db_test.go, line 1244 at r2 (raw file):

	fmt.Println(dir)
	require.NoError(t, err)
	// defer os.RemoveAll(dir)

Any reason to comment this out?


Comments from Reviewable

@manishrjain
Copy link
Contributor Author

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


db_test.go, line 1244 at r2 (raw file):

Previously, deepakjois (Deepak Jois) wrote…

Any reason to comment this out?

DONE. Left during testing.


iterator.go, line 89 at r1 (raw file):

Previously, peterstace (Peter Stace) wrote…

Would be good to indicate in the comment how dst is used. A user would have to read the code to know that dst will be written to starting at dst[0] rather than being appended to.

Done.


Comments from Reviewable

@deepakjois
Copy link
Contributor

Looks like this has been committed as: b3568eb

@deepakjois deepakjois closed this Nov 13, 2017
@deepakjois deepakjois deleted the mrjn/valuecopy branch November 13, 2017 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants