Make SafeDelete faster for large blocks; TSDB block repair case by jjneely · Pull Request #1056 · thanos-io/thanos

jjneely · 2019-04-18T19:21:28Z

This makes SafeDelete faster and less error prone when dealing with large blocks. This prevents SafeDelete from re-downloading a block that was just downloaded.

This PR also addresses a TSDB block repair case when duplicate series are detected.

Changes

SafeDelete takes another argument to use a pre-existing temporary directory possibly containing a previsouly downloaded TSDB block.
Duplicate series in a TSDB block are now handled when rewriting a block.

Verification

I run these patches in production, and have repaired quite a few hundred gig of blocks with these.

When we have label sets that are not in the correct order, fixing that changes the order of the series in the index. So the index must be rewritten in that new order. This makes this repair tool take up a bunch more memory, but produces blocks that verify correctly.

The directory name must be the block ID name exactly to verify. A temp directory or random name will not work here.

Pointer/reference logic error was eliminating all chunks for a series in a given TSDB block that wasn't the first chunk. Chunks are now referenced correctly via pointers.

If we have just downloaded a TSDB block for repair, we create a brand new block that is fixed. The original block can be used as the data we upload in SafeDelete() to save time/bandwidth rather than re-download it from the storage bucket.

Some linters catch errors.Errorf() as its not really part of the errors package.

…y/safe-delete

Where duplicate series means two series with an identical label set. This code just drops any duplicate series that is encountered after the first. No merging or anything fancy.

We're comparing items by pointers, using Go's range variables is misleading here and we need not fall into the same trap.

This prevents us from having to re-implement label sorting.

…y/safe-delete

jjneely · 2019-04-23T17:48:06Z

pkg/block/index.go

 	if err := metadata.Write(logger, resdir, &resmeta); err != nil {
 		return resid, err
 	}
+	// TSDB may rewrite metadata in bdir


The TSDB code handily rewrites the block metadata even though we don't write or change read block here. This nukes the extra metadata Thanos uses. Write it back.

So this is also a bug that you are trying to fix? Perhaps we should add it to the changelog and/or the PR messsage.

Not a bug, this is part of the initial patches to be able to reuse downloaded blocks. The TSDB library isn't aware of the additional meta data Thanos provides and was removing it. Therefore, when a previously downloaded block is reused it would have incomplete metadata without this change.

So this is being changed now: prometheus-junkyard/tsdb#637 so this won't be needed TBH (once we use latest TSDB).

Also this is true what @GiedriusS said - splitting PRs into focused changes helps a lot. However this change relies on this fix. I would be more explicit in description. This will help us to understand this early on (:

Let's add TODO to update TSDB and remove this as it will be not needed in newest TSDB. For now we need this, thanks for fixing this 👍

Todo added, thanks!

jjneely · 2019-04-23T17:50:04Z

pkg/block/index.go

+	lastSet := labels.Labels{}
 	// Build a new TSDB block.
 	for _, s := range series {
+		if labels.Compare(lastSet, s.lset) == 0 {


The series are sorted based on their labelSet. What happens if two labelSets are identical? For one they will be consecutive in the slice. So if the labelSet we have now is the same as the previous labelSet we have a duplicate series which the TSDB code will produce an error on. So we just drop that series.

Let's add a comment on the implied behavior here. I believe you when you say you've seen this behavior, but it worries me, as something went terribly wrong wherever these blocks were produced. We should at least document that the heuristic here is that on collision we keep the first series found and drop consecutive ones.

Done. Thank you!

Sorry one more thing. As this shouldn't generally happen, let's add a metric so we can monitor when it does happen.

The repair process batch job only which makes metrics less useful. I had some similar comments in #964

Perhaps I should leave a comment if that were to change?

Yes let's in that case put a TODO in there

TODO added.

jjneely · 2019-04-23T17:51:31Z

pkg/verifier/safe_delete.go

-	tempdir, err := ioutil.TempDir("", "safe-delete")
-	if err != nil {
-		return err
+	if tempdir == "" {


If we already have the block, don't download it again. Blocks can be large. 30GiB or close to it in my environment.

jjneely · 2019-04-26T18:50:24Z

I've tested this several times on my own TSDB blocks. I've been wondering how to get better tests integrated into the code as unit tests, but I cannot make these TSDB blocks of mine public. But I've repaired several hundred GiB of data with this.

brancz · 2019-04-29T08:45:56Z

pkg/verifier/safe_delete.go

+// source bucket.  It returns error if block dir already exists in backup
+// bucket (blocks should be immutable) or any of the operation fails.  If
+// the block has already been downloaded it must exist in tempdir, else it
+// will be downloaded to be copied to the backup bucket.  tempdir set to the


I'd prefer not to hide this behavior in tempdir values. Maybe "safe delete modes" (as in "force download", "try local copy") or a bool (as in just "forceDownload") at the very least that makes it very explicit what's going to happen.

SafeDelete() doesn't know the temporary directory where a previous function may have downloaded the block, so we need to pass in a path somehow. Perhaps rename this argument something like useExisting? Which would mean we use the existing block if found under that path, otherwise a new temporary directory is created and the block is downloaded.

sorry I meant let's pass two parameters to explicitly describe the intention

Ok, committed a patch for makes this explicit. I've tested the repair process with my collection of broken blocks I've started keeping and had the behavior expected. (No repeated downloads.)

Had thanos-compact cut a 450GiB TSDB block today which I referenced in #1152 which is part of the motivation for this.

brancz · 2019-04-29T08:53:27Z

pkg/verifier/safe_delete.go

+
+	if _, err := os.Stat(dir); err != nil {
+		// TSDB block not already present, download it
+		err = os.Mkdir(dir, 0755)


this should only be done if os.IsNotExist(err)

Fixed, thanks!

brancz · 2019-04-29T08:55:13Z

pkg/verifier/safe_delete.go

+			return errors.Wrap(err, "download from source")
+		}
+	} else {
+		level.Info(logger).Log("msg", "using previously downloaded tsdb block",


In what kind of situation would we end up in this path? And if the answer is a (half or maybe even fully) finished previous download, then how do we prevent that the directory is there but the content is not?

My blocks (and I'm sure others) easily grow above 30GiB when compacted. So if I need to repair block A, Thanos will download block A, create a repaired block B, upload block B. So at this point, we've created, successfully, a new block from A and that block B now passes the VerifyIndex() test.

At this point we call SafeDelete() which downloads block A, uploads block A to the backup bucket, then deletes block A from the main bucket.

This process can be time consuming (and perhaps expensive). So if blocks are immutable, and the repair process verifies that A is a legitimate block (if malformed) we should be able to skip the redundant download of A and backup the existing downloaded copy.

brancz · 2019-04-29T08:59:22Z

pkg/block/index.go

+	lastSet := labels.Labels{}
 	// Build a new TSDB block.
 	for _, s := range series {
+		if labels.Compare(lastSet, s.lset) == 0 {


Let's add a comment on the implied behavior here. I believe you when you say you've seen this behavior, but it worries me, as something went terribly wrong wherever these blocks were produced. We should at least document that the heuristic here is that on collision we keep the first series found and drop consecutive ones.

brancz · 2019-05-02T09:36:50Z

pkg/verifier/safe_delete.go

-		if err := os.RemoveAll(tempdir); err != nil {
-			level.Warn(logger).Log("msg", "failed to delete dir", "dir", tempdir, "err", err)
+
+	if _, err := os.Stat(dir); os.IsNotExist(err) {


we're swallowing any error that is not "IsNotExist" here

I think we should just do here os.Mkdir and ignore the error if it is saying that the directory already exists. Otherwise, we will have racy behaviour here (of course, there's barely any chance that someone will run two of these at the same time but still)

Yeah, good suggestions!

I added some additional logic here to handle any other possible error from os.Stat().

The pre-existence of the directory triggers the code path where we can avoid having to re-download a possibly large TSDB block again. Races with the os.Mkdir() are not anticipated as if the temporary directory isn't supplied ioutil.TempDir()is used to generate a value. When we do supply tempdir in pkg/verifier/index_issue.go is it also generated with ioutil.Tempdir():

tmpdir, err := ioutil.TempDir("", fmt.Sprintf("index-issue-block-%s-", id))

When we want to SafeDelete an already downloaded TSDB block, be very explicit about that use case.

brancz · 2019-05-17T12:35:10Z

pkg/verifier/safe_delete.go

+// previously downloaded block found in tempdir to avoid repeated downloads.
+// If tempdir is an empty string a unique temporary directory is created for
+// scratch space, otherwise the given directory is used.
+func SafeDelete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, backupBkt objstore.Bucket, id ulid.ULID, useExisting bool, tempdir string) error {


sorry for nitpicking on this, but can we name this useExistingTempdir? I realize that's long, but we'll understand in 3 months what this means that way :)

Not a problem. Fixed.

brancz · 2019-05-17T12:38:46Z

pkg/verifier/safe_delete.go

-	if err := block.Download(ctx, logger, bkt, id, dir); err != nil {
-		return errors.Wrap(err, "download from source")
+	if useExisting {
+		if _, err := os.Stat(filepath.Join(dir, "meta.json")); err != nil {


Is this check really sufficient? Shouldn't we check that all the downloaded content is equal to what is in object storage, ideally by their hash?

There don't appear to be tools for this in the existing code base. So this seems like a bit of scope creep. Glad to add additional checks here, but these features sound like another GitHub Issue/PR.

In fact, why isn't there a checksum in meta.json? It doesn't have to be cryptographically secure, but it would serve to verify that the block was intact. As we move from "metric data is ephemeral" to "metric data is important long term" that would be quite handy.

brancz · 2019-05-17T12:41:47Z

pkg/verifier/safe_delete.go

-	tempdir, err := ioutil.TempDir("", "safe-delete")
-	if err != nil {
-		return err
+	if tempdir == "" {


Maybe I wasn't clear, I would expect that when useExisting is set, that tempDir must be set, so I would expect an error from this function if that is not the case, and this if statement to be combined with the parts below, I think that'll also make the code a lot more easy to understand.

Ok, refactored this a bit. It will return an error if you ask it to use an existing tempdir and do not provide one, and the logic is a little simpler.

Be extra explicit about using existing tempdirs. Added note about future improvements for verifying if we have an intact tsdb block.

jjneely · 2019-05-17T20:03:56Z

cluster_test.go:129 fails on me again...

brancz · 2019-05-29T08:55:52Z

gossip has been removed, if you rebase I think those flakes should go away :)

brancz · 2019-05-29T08:56:46Z

Sorry for the silence, I'm working off the backlog that accumulated over kubecon, once rebased and CI is green I'm happy to give this another pass

jjneely · 2019-05-29T13:50:17Z

No problem. I know there are a lot of PRs as well. Does rebasing help you move this forward?

brancz · 2019-05-29T14:19:51Z

The test that kept flaking is gone due to the rebase, that's all :)

jjneely · 2019-06-07T13:50:30Z

Updates on getting this merged?

brancz

lgtm, but as this is easy to miss something I'd be good if we can get a second approval from @bwplotka or @GiedriusS

GiedriusS · 2019-06-14T12:30:48Z

pkg/block/index.go

+		if labels.Compare(lastSet, s.lset) == 0 {
+			level.Warn(logger).Log("msg",
+				"dropping duplicate series in tsdb block found",
+				"labelset", fmt.Sprintf("%s", s.lset),


You should use s.lset.String() here. The new meta linter will complain about this 😄

GiedriusS · 2019-06-14T12:31:38Z

pkg/block/index.go

 	if err := metadata.Write(logger, resdir, &resmeta); err != nil {
 		return resid, err
 	}
+	// TSDB may rewrite metadata in bdir


So this is also a bug that you are trying to fix? Perhaps we should add it to the changelog and/or the PR messsage.

GiedriusS · 2019-06-14T12:33:27Z

pkg/verifier/safe_delete.go

+// previously downloaded block found in tempdir to avoid repeated downloads.
+// If tempdir is an empty string a unique temporary directory is created for
+// scratch space, otherwise the given directory is used.
+func SafeDelete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, backupBkt objstore.Bucket, id ulid.ULID, useExistingTempdir bool, tempdir string) error {


Couldn't we just get rid of useExistingTempdir here and use tempdir to control how this function works? It seems like we are making this unnecessarily complex. Perhaps I am missing something.

I felt like giving a particular value of tempdir special meaning was not obvious enough. I think I prefer this over tempdir being empty. I do agree that below the if doesn't need the tempdir == "" case.

If you feel strongly about this @GiedriusS I'm ok with the other behavior, just feel I will have forgotten the implicit behavior of the empty string in 3 months, hence I wanted it to be more explicit.

Hm, I feel strongly about this - boolean in random place of the functions IMO smells bad. Same with 2 flags controlling same thing in very strict way.Essentially it does not make sense to set useExistingTempdir = true and tempdir = "" so we need to validate that etc. Interface is broader then needed.

Also if userExistingTempdir = true and tempdir is specified you literally don't use bkt so it's even worse in terms of readability and surprises ):

I would propose:

BackupAndDelete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, backupBkt objstore.Bucket, id ulid.ULID) as it was.

BackupAndDeleteFromLocal(ctx context.Context, logger log.Logger, bdir string, backupBkt objstore.Bucket)

This will ensure separation of concerns AND in fact BackupAndDelete can use BackupAndDeleteFromLocal internally. Right now what I can see is 2 function baked in one function (:

Yeah I like @bwplotka's suggestion best as well.

GiedriusS · 2019-06-14T12:34:21Z

pkg/verifier/safe_delete.go

-	err = os.Mkdir(dir, 0755)
-	if err != nil {
-		return err
+	if useExistingTempdir && tempdir == "" {


It seems like they both can be set or unset which proves my point.

I touched up the comments for this function again so that they better describe the behavior of the current code. No code changes though.

I proposed different approach - splitting this into 2 function which will solve this issue (:

This is now committed. The function signatures didn't quite work out as we would have liked, but I totally aggree, this feels much, much nicer.

The tempdir value is ignored if useExistingTempdir is false.

jjneely · 2019-06-24T14:45:25Z

Updates?

bwplotka

@jjneely Thanks for this!

I think it makes a lot of sense, but propose a split in SafeDelete to ensure readability and address @GiedriusS concerns (: Let me know if that makes sense.

Otherwise LGTM!

bwplotka · 2019-06-24T20:21:10Z

pkg/block/index.go

 	if err := metadata.Write(logger, resdir, &resmeta); err != nil {
 		return resid, err
 	}
+	// TSDB may rewrite metadata in bdir


So this is being changed now: prometheus-junkyard/tsdb#637 so this won't be needed TBH (once we use latest TSDB).

Also this is true what @GiedriusS said - splitting PRs into focused changes helps a lot. However this change relies on this fix. I would be more explicit in description. This will help us to understand this early on (:

bwplotka · 2019-06-24T20:21:35Z

pkg/block/index.go

 	if err := metadata.Write(logger, resdir, &resmeta); err != nil {
 		return resid, err
 	}
+	// TSDB may rewrite metadata in bdir


Also missing trailing period in comment.

bwplotka · 2019-06-24T20:22:22Z

pkg/block/index.go

+	lastSet := labels.Labels{}
 	// Build a new TSDB block.
 	for _, s := range series {
+		// The TSDB library will throw an error if we add a series with


Can we remove double spaces in this comment?

Old habits die hard...fixed.

Can I ask where is this habit from? Sorry for my ignorance - super curious (:

Lol... Search google for "double space after period" you'll learn more than you bargained for. Like, my age. :-)

TIL never heard of this either. But actually the rule could apply here since code formatting is using monospaced fonts mostly :)

bwplotka · 2019-06-24T20:35:47Z

pkg/block/index.go

 	if err := metadata.Write(logger, resdir, &resmeta); err != nil {
 		return resid, err
 	}
+	// TSDB may rewrite metadata in bdir


Let's add TODO to update TSDB and remove this as it will be not needed in newest TSDB. For now we need this, thanks for fixing this 👍

pkg/verifier/safe_delete.go

bwplotka · 2019-06-24T20:46:45Z

pkg/verifier/safe_delete.go

+// previously downloaded block found in tempdir to avoid repeated downloads.
+// If tempdir is an empty string a unique temporary directory is created for
+// scratch space, otherwise the given directory is used.
+func SafeDelete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, backupBkt objstore.Bucket, id ulid.ULID, useExistingTempdir bool, tempdir string) error {


Hm, I feel strongly about this - boolean in random place of the functions IMO smells bad. Same with 2 flags controlling same thing in very strict way.Essentially it does not make sense to set useExistingTempdir = true and tempdir = "" so we need to validate that etc. Interface is broader then needed.

Also if userExistingTempdir = true and tempdir is specified you literally don't use bkt so it's even worse in terms of readability and surprises ):

I would propose:

BackupAndDelete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, backupBkt objstore.Bucket, id ulid.ULID) as it was.

BackupAndDeleteFromLocal(ctx context.Context, logger log.Logger, bdir string, backupBkt objstore.Bucket)

This will ensure separation of concerns AND in fact BackupAndDelete can use BackupAndDeleteFromLocal internally. Right now what I can see is 2 function baked in one function (:

bwplotka · 2019-06-24T20:47:44Z

pkg/verifier/safe_delete.go

-	err = os.Mkdir(dir, 0755)
-	if err != nil {
-		return err
+	if useExistingTempdir && tempdir == "" {


I proposed different approach - splitting this into 2 function which will solve this issue (:

bwplotka · 2019-06-24T20:50:43Z

And sorry for massive delay in review!

Co-Authored-By: Bartek Płotka <bwplotka@gmail.com>

bwplotka

Happy to put LGTM once we address the https://github.com/improbable-eng/thanos/pull/1056/files#r296908203 (:

bwplotka · 2019-06-25T22:12:44Z

pkg/block/index.go

 		return resid, err
 	}
+	// TSDB may rewrite metadata in bdir.
+	// TODO: This is not needed in newer TSDB code.


PR number would be nice for easier trackngm but not a blocker (:

bwplotka · 2019-06-25T22:13:25Z

pkg/block/index.go

+	lastSet := labels.Labels{}
 	// Build a new TSDB block.
 	for _, s := range series {
+		// The TSDB library will throw an error if we add a series with


Can I ask where is this habit from? Sorry for my ignorance - super curious (:

bwplotka

One more nit and LGTM, thanks for this!

bwplotka · 2019-06-28T15:41:28Z

pkg/verifier/safe_delete.go

+		return errors.Errorf("%s dir seems to exists in backup bucket. Remove this block manually if you are sure it is safe to do", id)
+	}
+
+	// Now we call backupAndDeleteDownloaded() to handle the work.


I think this comment is not needed.

bwplotka · 2019-06-28T15:47:45Z

pkg/verifier/safe_delete.go

+// backupAndDeleteDownloaded is a helper function that uploads a TSDB block
+// found on disk to the backup bucket and, on success, removes the TSDB block
+// from the source bucket.
+func backupAndDeleteDownloaded(ctx context.Context, logger log.Logger, bdir string, bkt, backupBkt objstore.Bucket, id ulid.ULID) error {


Sorry for being picky but there is some improvement we can make here.

I like BackupAndDelete vs BackupAndDeleteDownloaded, but it's not clear what's the difference between backupAndDeleteDownloaded and BackupAndDeleteDownloaded is. Sure they are heavily commented, but comment should have less priority than naming. I think here the proper naming and small possible signature of function would do the work.

So what about:

Suggested change

func backupAndDeleteDownloaded(ctx context.Context, logger log.Logger, bdir string, bkt, backupBkt objstore.Bucket, id ulid.ULID) error {

func backupDownloaded(ctx context.Context, logger log.Logger, bdir string, bktToRemove, backupBkt objstore.Bucket, id ulid.ULID) error {

and then if no error do on the caller in both BackupAndDeleteDownloaded and BackupAndDelete just.

// Block uploaded, so we are ok to remove from src bucket. if err := block.Delete(ctx, bkt, id); err != nil { return errors.Wrap(err, "delete from source") }

This will make the signature of BackupAndDeleteDownloaded and backupDowloaded smaller (thus reducing complexity) as you can drop bkt argument.

What do you think? (:

Adjusted as requested, and went through another round of testing.

bwplotka

Awesome! Thanks 👍

bwplotka · 2019-07-08T12:04:27Z

I think netlify issue is because of not rebased PR - nothing changed for netlify here so merging.

jjneely · 2019-07-08T13:30:45Z

Awesome, thanks!

jjneely added 13 commits March 25, 2019 12:08

bucket verify: repair out of order labels

f0dfe27

Fix the TSDB block safe-delete function

230bc7b

The directory name must be the block ID name exactly to verify. A temp directory or random name will not work here.

verify repair: fix duplicate chunk detection

3f0d6cd

Pointer/reference logic error was eliminating all chunks for a series in a given TSDB block that wasn't the first chunk. Chunks are now referenced correctly via pointers.

PR feedback: use errors.Errorf() instead of fmt.Errorf()

ed78bde

Use errors.New()

2f179ff

Some linters catch errors.Errorf() as its not really part of the errors package.

Merge branch 'jjneely/fix-repair-for-out-of-order-labels' into jjneel…

9ca0c05

…y/safe-delete

Repair duplicate series in a TSDB block

fd572aa

Where duplicate series means two series with an identical label set. This code just drops any duplicate series that is encountered after the first. No merging or anything fancy.

Liberally comment this for loop

24173d3

We're comparing items by pointers, using Go's range variables is misleading here and we need not fall into the same trap.

Take advantage of sort.Interface

0d39253

This prevents us from having to re-implement label sorting.

Merge branch 'jjneely/fix-repair-for-out-of-order-labels' into jjneel…

5a7fd5d

…y/safe-delete

PR Feedback: Comments are full sentences.

bfd44f1

jjneely force-pushed the jjneely/safe-delete-pr branch from 54b0722 to 8225c79 Compare April 18, 2019 19:30

jjneely added 2 commits April 23, 2019 13:26

Merge branch 'jjneely/fix-repair-for-out-of-order-labels' into jjneel…

d1274ef

…y/safe-delete

Merge branch 'jjneely/safe-delete' into jjneely/safe-delete-pr

0a01898

jjneely force-pushed the jjneely/safe-delete-pr branch from 8225c79 to 0a01898 Compare April 23, 2019 17:36

jjneely commented Apr 23, 2019

View reviewed changes

brancz reviewed Apr 29, 2019

View reviewed changes

PR Feedback

bf62c5c

brancz reviewed May 2, 2019

View reviewed changes

jjneely added 3 commits May 13, 2019 14:50

PR Feedback: Handle other possible Stat() errors

e296f8f

PR Feedback: Add TODO for future metric of dropped series in block

768ec93

PR feedback: Make SafeDelete() more explicit

c46ff1c

When we want to SafeDelete an already downloaded TSDB block, be very explicit about that use case.

brancz reviewed May 17, 2019

View reviewed changes

PR Feedback: refactor SafeDelete

b625a95

Be extra explicit about using existing tempdirs. Added note about future improvements for verifying if we have an intact tsdb block.

brancz approved these changes Jun 7, 2019

View reviewed changes

GiedriusS reviewed Jun 14, 2019

View reviewed changes

jjneely added 2 commits June 14, 2019 14:26

Use lset.String() rather than fmt.Sprintf()

380672b

SafeDelete(): Make comments match function behavior

5599139

The tempdir value is ignored if useExistingTempdir is false.

bwplotka requested changes Jun 24, 2019

View reviewed changes

jjneely and others added 4 commits June 25, 2019 15:54

PR feedback: comments in pkg/verifier/safe_delete.go

e6ad6a0

Co-Authored-By: Bartek Płotka <bwplotka@gmail.com>

PR feedback: comments in pkg/verifier/safe_delete.go

649df4d

Co-Authored-By: Bartek Płotka <bwplotka@gmail.com>

PR Feedback: comments in pkg/block/index.go

9e26cfd

Update pkg/verifier/safe_delete.go

31f7dd6

Co-Authored-By: Bartek Płotka <bwplotka@gmail.com>

bwplotka reviewed Jun 25, 2019

View reviewed changes

jjneely added 2 commits June 27, 2019 16:11

PR feedback: split SafeDelete() into multiple functions

f6f3443

PR Feedback: Log the upstream PR with this TODO

37d495a

bwplotka reviewed Jun 28, 2019

View reviewed changes

PR Feedback: clean up function signatures

b852c34

brancz approved these changes Jul 8, 2019

View reviewed changes

bwplotka approved these changes Jul 8, 2019

View reviewed changes

bwplotka merged commit 63257cf into thanos-io:master Jul 8, 2019

	func backupAndDeleteDownloaded(ctx context.Context, logger log.Logger, bdir string, bkt, backupBkt objstore.Bucket, id ulid.ULID) error {
	func backupDownloaded(ctx context.Context, logger log.Logger, bdir string, bktToRemove, backupBkt objstore.Bucket, id ulid.ULID) error {

Conversation

jjneely commented Apr 18, 2019

Changes

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjneely commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjneely commented May 17, 2019

Uh oh!

brancz commented May 29, 2019