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.

Sign up for GitHub

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 two query API bugs #521

Merged
merged 1 commit into from
Sep 6, 2018
Merged

Fix two query API bugs #521

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 49 additions & 6 deletions api/query/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,21 +64,59 @@ func (gz gzipReadWritable) NewReadCloser(id string) (io.ReadCloser, error) {
if err != nil {
return nil, err
}
return gzip.NewReader(r)

gzr, err := gzip.NewReader(r)
if err != nil {
return nil, err
}

return compositeReadWriteCloser{
reader: gzr,
owner: gzr,
delegate: r,
}, nil
}

func (gz gzipReadWritable) NewWriteCloser(id string) (io.WriteCloser, error) {
w, err := gz.delegate.NewWriteCloser(id)
if err != nil {
return nil, err
}
return gzip.NewWriter(w), nil

gzw := gzip.NewWriter(w)
return compositeReadWriteCloser{
writer: gzw,
owner: gzw,
delegate: w,
}, nil
}

type memcacheReadWritable struct {
ctx context.Context
}

type compositeReadWriteCloser struct {
reader io.Reader
writer io.Writer
owner io.Closer
delegate io.Closer
}

func (crwc compositeReadWriteCloser) Read(p []byte) (n int, err error) {
return crwc.reader.Read(p)
}

func (crwc compositeReadWriteCloser) Write(p []byte) (n int, err error) {
return crwc.writer.Write(p)
}

func (crwc compositeReadWriteCloser) Close() error {
if err := crwc.owner.Close(); err != nil {
return err
}
return crwc.delegate.Close()
}

type memcacheWriteCloser struct {
memcacheReadWritable
key string
Expand All @@ -97,17 +135,17 @@ func (mc memcacheReadWritable) NewReadCloser(key string) (io.ReadCloser, error)
}

func (mc memcacheReadWritable) NewWriteCloser(key string) (io.WriteCloser, error) {
Copy link
Member

Choose a reason for hiding this comment

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

@mdittmer I know you've merged the PR but still want to share a thought on this:

The fact that changing the return value doesn't change the function signature reminds me of a Go convention that we didn't follow here: take interfaces as arguments, and return concrete types. In this example, we should return *memcacheWriteCloser. Explicitness in return values benefits the callers (and callers don't have to care - they can always use := or even assign the return value to an interface type), while the polymorphism in arguments makes the function more generic.

return memcacheWriteCloser{mc, key, bytes.Buffer{}, false}, nil
return &memcacheWriteCloser{mc, key, bytes.Buffer{}, false}, nil
}

func (mw memcacheWriteCloser) Write(p []byte) (n int, err error) {
func (mw *memcacheWriteCloser) Write(p []byte) (n int, err error) {
if mw.isClosed {
return 0, errMemcacheWriteCloserWriteAfterClose
}
return mw.b.Write(p)
}

func (mw memcacheWriteCloser) Close() error {
func (mw *memcacheWriteCloser) Close() error {
mw.isClosed = true
return memcache.Set(mw.ctx, &memcache.Item{
Key: mw.key,
Expand Down Expand Up @@ -196,10 +234,15 @@ func (cs cachedStore) Get(cacheID, storeID string) ([]byte, error) {
logger.Warningf("Error cache writer for key %s: %v", cacheID, err)
}
}()
if _, err := w.Write(data); err != nil {
n, err := w.Write(data)
if err != nil {
logger.Warningf("Failed to write to cache key %s: %v", cacheID, err)
return
}
if n != len(data) {
logger.Warningf("Failed to write to cache key %s: attempt to write %d bytes, but wrote %d bytes instead", cacheID, len(data), n)
return
}
}()

return data, nil
Expand Down