compaction: remove all cache dirs at the end of each run by krasi-georgiev · Pull Request #1587 · thanos-io/thanos

krasi-georgiev · 2019-09-30T14:13:33Z

Signed-off-by: Krasi Georgiev 8903888+krasi-georgiev@users.noreply.github.com
fixes: #1499

TODO

tests

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka

Some questions (:

CHANGELOG.md

bwplotka · 2019-10-02T13:32:37Z

cmd/thanos/compact.go

 		indexCacheDir   = path.Join(dataDir, "index_cache")
 	)

-	if err := os.RemoveAll(downsamplingDir); err != nil {


Can we not remove it? I think we need that still. What if compactor crashes/is restarted not gracefully? Then deferred removal will fail and Thanos will use more disk then needed if not worse - might end up having corrupted blocks.

this is not removed , it is just moved inside the .Compact() method. This makes it consitent with all the how the other components handle removing the tmp files.

actually I just double checked and I removed it because it is redundant as the compactor removes these before every compaction in

thanos/pkg/compact/compact.go

Lines 1044 to 1047 in 3646e13

// Clean up the compaction temporary directory at the beginning of every compaction loop.

if err := os.RemoveAll(c.compactDir); err != nil {

return errors.Wrap(err, "clean up the compaction temporary directory")

}

Yes, but c.compactDir is different then downsamplingDir 🤔

correction It is a duplicate of

thanos/cmd/thanos/downsample.go

Lines 134 to 136 in 95757ea

if err := os.RemoveAll(dir); err != nil {

return errors.Wrap(err, "clean working directory")

}

Yes but that would be cleaned only after (!) compaction, so long time. I am 99% sure we need to keep this.

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev

I have now added tests which verify the correct behavior.

krasi-georgiev · 2019-10-02T21:53:43Z

cmd/thanos/compact.go

 		indexCacheDir   = path.Join(dataDir, "index_cache")
 	)

-	if err := os.RemoveAll(downsamplingDir); err != nil {


this is not removed , it is just moved inside the .Compact() method. This makes it consitent with all the how the other components handle removing the tmp files.

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2019-10-04T08:36:40Z

ping @bwplotka

bwplotka · 2019-10-04T09:48:58Z

cmd/thanos/main_test.go

+	"github.com/thanos-io/thanos/pkg/testutil"
+)
+
+func TestCleanupCompactCacheFolder(t *testing.T) {


Why not having those inside package responsible for deleting?

because it will require duplicating the bootstrap function.
I thought to put the bootstrap in some reusable package, but don't think it is generic enough to be useful anywhere else.
If it happens that that func becomes useful in other tests would revisit and refactor.

bwplotka · 2019-10-04T09:49:44Z

cmd/thanos/compact.go

 		indexCacheDir   = path.Join(dataDir, "index_cache")
 	)

-	if err := os.RemoveAll(downsamplingDir); err != nil {


Yes, but c.compactDir is different then downsamplingDir 🤔

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

# Conflicts: # CHANGELOG.md Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

* remove all cache dirs at the end of each run Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * changelog Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * . Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * changelog formating and pr num. Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * added test for the compaction cleanup Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * added tests Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * check error Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * move GatherAndCompare to tesutil Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * comment Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * nit Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * Added back deletion of the downsampling dir. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Fix after pull master. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

krasi-georgiev added 6 commits September 30, 2019 17:07

remove all cache dirs at the end of each run

95757ea

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

changelog

acf199e

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

.

3fe3fe5

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

changelog formating and pr num.

9c57518

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into cleanup-temp-files

4914be4

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

added test for the compaction cleanup

bb1610f

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka reviewed Oct 2, 2019

View reviewed changes

added tests

3646e13

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev commented Oct 2, 2019

View reviewed changes

krasi-georgiev added 4 commits October 3, 2019 03:41

check error

bd7f4fe

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

move GatherAndCompare to tesutil

74fad16

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

comment

808d83b

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

nit

203ed41

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka reviewed Oct 4, 2019

View reviewed changes

Added back deletion of the downsampling dir.

3c678b2

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka force-pushed the cleanup-temp-files branch from a08d04b to 3c678b2 Compare October 10, 2019 10:58

bwplotka added 2 commits October 10, 2019 12:02

Merge branch 'master' into krasi-georgiev-cleanup-temp-files

5682646

# Conflicts: # CHANGELOG.md Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Fix after pull master.

c326cc8

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka approved these changes Oct 10, 2019

View reviewed changes

bwplotka merged commit a09a4b9 into thanos-io:master Oct 10, 2019

krasi-georgiev deleted the cleanup-temp-files branch October 10, 2019 11:56

	// Clean up the compaction temporary directory at the beginning of every compaction loop.
	if err := os.RemoveAll(c.compactDir); err != nil {
	return errors.Wrap(err, "clean up the compaction temporary directory")
	}

	if err := os.RemoveAll(dir); err != nil {
	return errors.Wrap(err, "clean working directory")
	}

Conversation

krasi-georgiev commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

krasi-georgiev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev commented Oct 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

krasi-georgiev commented Sep 30, 2019 •

edited

Loading

krasi-georgiev Oct 3, 2019 •

edited

Loading