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

extract common cache with usability improvements #6

Merged
merged 14 commits into from Mar 27, 2019
Merged

Conversation

noamnelke
Copy link
Member

No description provided.

@noamnelke noamnelke requested a review from zalmen March 20, 2019 20:53
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/layerfactories.go Outdated Show resolved Hide resolved
cache/slice.go Outdated
return value, nil
}

func (s *SliceReadWriter) Write(p []byte) (n int, err error) {
Copy link

Choose a reason for hiding this comment

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

I would expect that a method called "Write" will write to where position is pointing to, are you sure that the expected behavior is to append the data?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a little confusing, I agree. Not sure how to fix it though.

Write is the Writer part, while the other methods are the Reader part. They are independent and position is only relevant to the Reader part. Write is append-only.

I'm not sure what's the best way to keep this behaviour and also make it clear to an uninformed user. If I change the function name to Append that would break the io.Writer interface implementation, but maybe that's the best solution. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't be the first to have a combined read-writer that actually work independently. See go's bufio.ReadWriter -- its reader and writer work in isolation and don't share the position with each other.

I'm leaving as-is, unless you tell me otherwise.

Copy link

Choose a reason for hiding this comment

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

Rename Write to Append and add a comment next to Seek saying that it only effects the reader

proving.go Show resolved Hide resolved
@noamnelke noamnelke requested a review from zalmen March 25, 2019 20:07
zalmen
zalmen previously approved these changes Mar 26, 2019
cache/slice.go Outdated
return value, nil
}

func (s *SliceReadWriter) Write(p []byte) (n int, err error) {
Copy link

Choose a reason for hiding this comment

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

Rename Write to Append and add a comment next to Seek saying that it only effects the reader

cache/cache.go Outdated
}

func (c *Cache) LeafReader() LayerReadWriter {
return c.layers[0]

Choose a reason for hiding this comment

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

what if there are no layers?

Copy link
Member Author

Choose a reason for hiding this comment

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

This method doesn't exist anymore, but there's:

func (c *Reader) GetLayerReader(layerHeight uint) LayerReader {
	return c.layers[layerHeight]
}

So the answer is that the layers map is initialized in the constructor and requesting a non-existent layer is a legit scenario that happens normally. If you try to read from a map with a key that doesn't exist you get the zero value, nil in this case.

antonlerner
antonlerner previously approved these changes Mar 27, 2019
@noamnelke noamnelke merged commit 3651a54 into develop Mar 27, 2019
@noamnelke noamnelke deleted the cache_object branch March 27, 2019 14:54
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