compact: introduce flag --wait-sync-block-duration #2752

chrischdi · 2020-06-10T17:56:16Z

Thanos compact currently does metadata sync every minute if --wait is
active. Thanos store has a equivalent flag named sync-block-duration.
This commit introduces --wait-sync-block-duration to be able to
configure the interval via a flag.

Resolves #2642

Signed-off-by: Schlotter, Christian christi.schlotter@gmail.com

I added CHANGELOG entry for this change.

Changes

added flag --wait-sync-block-duration to compact command
used value of flag instead of hardcoded time.Minute

Verification

Run it in production

bwplotka

Awesome, this is important fix I agree.

BTW what do you think of something else - some logic of 0 were you disable syncing totally.

It would be only refreshed when refresh bucket UI page, WDYT?

In the mean time we can merge this 👍 one suggestion only.

Thanks! ❤️

bwplotka · 2020-06-18T16:15:19Z

cmd/thanos/compact.go

@@ -475,6 +476,8 @@ func (cc *compactConfig) registerFlag(cmd *kingpin.CmdClause) *compactConfig {
 		Short('w').BoolVar(&cc.wait)
 	cmd.Flag("wait-interval", "Wait interval between consecutive compaction runs and bucket refreshes. Only works when --wait flag specified.").
 		Default("5m").DurationVar(&cc.waitInterval)
+	cmd.Flag("wait-sync-block-interval", "Repeat interval for syncing the blocks between local and remote view.").


This is good but I think we have to name it better to show it's related to the UI.

What about something like:

Suggested change

cmd.Flag("wait-sync-block-interval", "Repeat interval for syncing the blocks between local and remote view.").

cmd.Flag("block-viewer.global.sync-block-interval", "Repeat interval for syncing the blocks between local and remote view for /global Block Viewer UI.").

Sure, I will fix that and also fix the changelog conflict :-) (will have to rebuild the docs anyway)

chrischdi · 2020-06-18T16:25:33Z

Awesome, this is important fix I agree.

BTW what do you think of something else - some logic of 0 were you disable syncing totally.

It would be only refreshed when refresh bucket UI page, WDYT?

In the mean time we can merge this one suggestion only.

Thanks!

Sounds also reasonable to me.
But maybe it would also need some logic to not re-execute syncing on every request maybe (use-case: opening the page multiple times by accident).

Maybe it would make more sense to allow using the cached index for the bucket UI?

bwplotka · 2020-06-18T16:28:14Z

Maybe it would make more sense to allow using the cached index for the bucket UI?

Yes, it's must have - in separate PR (:

BTW @pstibrany do we have to cache for Cortex compactor? I guess yes?

pstibrany · 2020-06-18T16:38:09Z

BTW @pstibrany do we have to cache for Cortex compactor? I guess yes?

In Cortex we don't use cache for compactor. Our default interval for running compactor is one hour, so it scans bucket only once per hour.

While we could use cache that is shared by queriers/store-gateways, but since scans are so infrequent, we decided that compactor having the correct picture of the world is more important.

pracucci · 2020-06-19T14:36:31Z

BTW @pstibrany do we have to cache for Cortex compactor? I guess yes?

In Cortex we don't use cache for compactor. Our default interval for running compactor is one hour, so it scans bucket only once per hour.

While we could use cache that is shared by queriers/store-gateways, but since scans are so infrequent, we decided that compactor having the correct picture of the world is more important.

I'm not sure it's safe. The compactor should always have the real state of the ~~world~~ bucket.

pracucci

LGTM (modulo a nit)

CHANGELOG.md

chrischdi · 2020-06-19T15:24:56Z

BTW @pstibrany do we have to cache for Cortex compactor? I guess yes?

In Cortex we don't use cache for compactor. Our default interval for running compactor is one hour, so it scans bucket only once per hour.
While we could use cache that is shared by queriers/store-gateways, but since scans are so infrequent, we decided that compactor having the correct picture of the world is more important.

I'm not sure it's safe. The compactor should always have the real state of the ~~world~~ bucket.

The compactor yes but the bucket web UI may not need to have that?!

pracucci · 2020-06-19T15:50:40Z

BTW @pstibrany do we have to cache for Cortex compactor? I guess yes?

In Cortex we don't use cache for compactor. Our default interval for running compactor is one hour, so it scans bucket only once per hour.
While we could use cache that is shared by queriers/store-gateways, but since scans are so infrequent, we decided that compactor having the correct picture of the world is more important.

I'm not sure it's safe. The compactor should always have the real state of the ~~world~~ bucket.

The compactor yes but the bucket web UI may not need to have that?!

The UI can use as far as you accept to not see the latest state of the bucket. I personally would prefer to have the UI to always reflect the latest state, but I'm open to discuss it.

chrischdi · 2020-06-20T11:17:37Z

Hi @pracucci / @bwplotka ,

I also fixed the nit :-)

Could it be that the thanos tests failed due to some flakyness? I thought they had been green before merging that nit.

Thanos compact currently does metadata sync every minute if `--wait` is active. Thanos store has a equivalent flag named `sync-block-duration`. This commit introduces `--block-viewer.global.sync-block-interval` to be able to configure the interval via a flag. Signed-off-by: Schlotter, Christian <christi.schlotter@gmail.com>

chrischdi · 2020-06-26T05:16:35Z

Rebased to fix CHANGELOG.md conflict

bwplotka

Awesome, thanks! 👍

bwplotka · 2020-06-26T09:43:58Z

cmd/thanos/compact.go

@@ -482,6 +483,8 @@ func (cc *compactConfig) registerFlag(cmd *kingpin.CmdClause) *compactConfig {

 	cmd.Flag("block-sync-concurrency", "Number of goroutines to use when syncing block metadata from object storage.").
 		Default("20").IntVar(&cc.blockSyncConcurrency)
+	cmd.Flag("block-viewer.global.sync-block-interval", "Repeat interval for syncing the blocks between local and remote view for /global Block Viewer UI.").
+		Default("1m").DurationVar(&cc.blockViewerSyncBlockInterval)


If it is causing problems, why not reducing this to a minimum? 🤔 Anyway we can think in later PRs!

* upstream/release-0.14: (46 commits) Cut release v0.14.0-rc.1 (thanos-io#2853) Query: correctly marshal errors to JSON and ignore if nil (thanos-io#2848) ci: Manually download promu in crossbuild stage (thanos-io#2828) Cut release v0.14.0-rc.0 (thanos-io#2826) Soft cut changelog on master to indicate v0.14.0 being in progress (thanos-io#2824) Update ThanosReceiveNoUpload to select sum == 0 (thanos-io#2819) receive: Added more observability, fixed leaktest, to actually check leaks ): (thanos-io#2817) Query: always return a string in the `lastError` field (thanos-io#2809) Added missing CHANGELOG entry for PR 2613 (thanos-io#2820) receive: Fixed small options race; Removed unused StartTime feature. (thanos-io#2816) go.mod: Bump Prometheus to current latest (thanos-io#2814) Implement CLI Flags page in React UI (thanos-io#2796) Improve ThanosReceiveNoUpload to only alert on current instances store: Preallocate output buffer when encoding postings. (thanos-io#2812) compact: introduce flag --block-viewer.global.sync-block-interval (thanos-io#2752) docs: compact: add blurb about how retention policy works (thanos-io#2808) Reduced memory allocations in readIndexRange() (thanos-io#2807) ui: Add Stores page to React UI (thanos-io#2754) Added Kemal to Maintainer Role; Kemal is volounteering to be next release shephard (thanos-io#2804) proposal: Add scalable rule storage proposal (thanos-io#2661) ...

chrischdi force-pushed the compact/add-wait-sync-block-duration-flag branch from c1b62fb to d681877 Compare June 10, 2020 17:57

chrischdi mentioned this pull request Jun 17, 2020

store recommends setting proper mlock limits which is not possible in upstream docker images #2774

Closed

bwplotka approved these changes Jun 18, 2020

View reviewed changes

chrischdi force-pushed the compact/add-wait-sync-block-duration-flag branch from d681877 to b38c6cd Compare June 19, 2020 05:42

pracucci approved these changes Jun 19, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chrischdi force-pushed the compact/add-wait-sync-block-duration-flag branch from 1a26214 to bc9a7a0 Compare June 20, 2020 07:35

chrischdi force-pushed the compact/add-wait-sync-block-duration-flag branch from bc9a7a0 to 1011c4b Compare June 26, 2020 05:16

bwplotka approved these changes Jun 26, 2020

View reviewed changes

bwplotka merged commit a7bb287 into thanos-io:master Jun 26, 2020

chrischdi deleted the compact/add-wait-sync-block-duration-flag branch June 26, 2020 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compact: introduce flag --wait-sync-block-duration #2752

compact: introduce flag --wait-sync-block-duration #2752

chrischdi commented Jun 10, 2020 •

edited

Loading

bwplotka left a comment

bwplotka Jun 18, 2020

chrischdi Jun 18, 2020

chrischdi commented Jun 18, 2020

bwplotka commented Jun 18, 2020

pstibrany commented Jun 18, 2020

pracucci commented Jun 19, 2020

pracucci left a comment

chrischdi commented Jun 19, 2020

pracucci commented Jun 19, 2020

chrischdi commented Jun 20, 2020

chrischdi commented Jun 26, 2020

bwplotka left a comment

bwplotka Jun 26, 2020

	cmd.Flag("wait-sync-block-interval", "Repeat interval for syncing the blocks between local and remote view.").
	cmd.Flag("block-viewer.global.sync-block-interval", "Repeat interval for syncing the blocks between local and remote view for /global Block Viewer UI.").

compact: introduce flag --wait-sync-block-duration #2752

compact: introduce flag --wait-sync-block-duration #2752

Conversation

chrischdi commented Jun 10, 2020 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Jun 18, 2020

Choose a reason for hiding this comment

chrischdi Jun 18, 2020

Choose a reason for hiding this comment

chrischdi commented Jun 18, 2020

bwplotka commented Jun 18, 2020

pstibrany commented Jun 18, 2020

pracucci commented Jun 19, 2020

pracucci left a comment

Choose a reason for hiding this comment

chrischdi commented Jun 19, 2020

pracucci commented Jun 19, 2020

chrischdi commented Jun 20, 2020

chrischdi commented Jun 26, 2020

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Jun 26, 2020

Choose a reason for hiding this comment

chrischdi commented Jun 10, 2020 •

edited

Loading