Add support for integration in GCP storage #62
Conversation
AlCutter
force-pushed
the
pr_integrate_gcp
branch
4 times, most recently
from
fbe3369 to
64ab0e8
Compare
AlCutter
force-pushed
the
pr_integrate_gcp
branch
from
64ab0e8 to
af22619
Compare
| if err := errG.Wait(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Does this cause a problem if the first error is NotExist but another one is a real failure?
There was a problem hiding this comment.
Technically it's "fine" because the only time when we'd expect to see NotExist tiles is after the prewarm, so we're only fetching one at a time. But it also sucks, so I've changed the API to be more friendly to the new multi-fetch shape of things. Now "NotExist" is represented as a nil entry in the slice, and errors are reserved for bad situations.
storage/gcp/gcp.go
Outdated
| if err := s.setEntryBundle(ctx, bundleIndex, fromSeq, &bundle); err != nil { | ||
| if !errors.Is(os.ErrExist, err) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
It could be worth logging something here when a duplicate write has happened. What I'd actually prefer would be prometheus counters for successful write, error write, duplicate write, etc. But that can come later?
There was a problem hiding this comment.
FWIW the GCP impl already logs a (verbose level) message when it encounters an idempotent dupe write, but agree that metrics are the right thing.
| }(ctx, k, v) | ||
| } | ||
| errG.Go(func() error { | ||
| //TODO: write out checkpoint |
There was a problem hiding this comment.
This is quite a load bearing TODO :-)
There was a problem hiding this comment.
Coming soon, to a PR near you! :)
This PR pulls in the integrate mix-in from #61 and uses it in the GCP storage.
Towards #23