Store-gateway: Add no-compact column on store-gateway/tenants admin UI #6848

ying-jeanne · 2023-12-07T15:58:44Z

What this PR does

This PR introduces a new filter 'no-compact' in the admin UI for block filtering. Currently, we have various filters available for markers, but this addition extends functionality by enabling the display of blocks marked with 'no-compact'. Subsequent PRs will build upon this enhancement to provide users with the capability to mark blocks directly through the UI instead of CLI.

Test in local, how it looks like on UI

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

na Tests updated.
na Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
na about-versioning.md updated with experimental features.

pstibrany · 2023-12-07T16:01:14Z

pkg/storegateway/blocks.gohtml

@@ -79,6 +82,14 @@
                    {{ end }}
                </td>
            {{ end }}
+            {{ if $page.ShowNoCompact }}


Let's always show no compact markers when listing blocks via web.

pstibrany · 2023-12-07T16:02:36Z

pkg/util/listblocks/listblocks.go

 		}
 	}

 	metas, err = fetchMetas(ctx, bkt, metaPaths)
-	return metas, deletionTimes, err
+	return metas, deletionTimes, noCompactBlocks, err


It would be nice to fetch "reasons" and timestamps from no-compact markers, and return those.

pkg/util/listblocks/listblocks.go

dimitarvdimitrov · 2023-12-08T13:22:39Z

pkg/util/listblocks/listblocks.go

-	return times, concurrency.ForEachJob(ctx, len(deletionMarkers), concurrencyLimit, func(ctx context.Context, idx int) error {
-		r, err := bkt.Get(ctx, deletionMarkers[idx])
+	return details, concurrency.ForEachJob(ctx, len(markers), concurrencyLimit, func(ctx context.Context, idx int) error {
+		r, err := bkt.Get(ctx, markers[idx])


can we reuse block.ReadMarker()? it looks like it's doing pretty much the same job as here

the function was there, i didn't look into make it different because of the change size, it is not really relevant to the PR title. I see the difference between listblocks and other cases we use ReadMarker, is that here we do execute the read in concurrency. I would prefer not to change that in this PR, if possible it could be a good general refectory task for following.

pkg/storage/tsdb/block/markers.go

pkg/storegateway/gateway_blocks_http.go

pstibrany

Left some post-merge comments.

pstibrany · 2023-12-11T09:09:54Z

pkg/util/listblocks/listblocks.go

 }

 const concurrencyLimit = 32

-func fetchDeletionTimes(ctx context.Context, bkt objstore.BucketReader, deletionMarkers []string) (map[ulid.ULID]time.Time, error) {
+func fetchMarkerDetails[MARKER_TYPE block.Marker](ctx context.Context, bkt objstore.BucketReader, markers []string) (map[ulid.ULID]MARKER_TYPE, error) {


All generic types are "types", there's no need to include that in the name. I'd suggest to shorten MARKER_TYPE to just M.

pstibrany · 2023-12-11T09:12:30Z

pkg/storegateway/gateway_blocks_http.go

@@ -149,10 +159,9 @@ func (s *StoreGateway) BlocksHandler(w http.ResponseWriter, req *http.Request) {
 	}, blocksPageTemplate, req)
 }

-func formatTimeIfNotZero(t time.Time, format string) string {
-	if t.IsZero() {
+func formatTimeIfNotZero(t int64, format string) string {


We should use strong types instead of "integers" for times. Perhaps we could use *time.Time in richMeta?

pstibrany · 2023-12-11T09:13:57Z

pkg/util/listblocks/listblocks.go

 		}
 	}
-
+	noCompactDetails, err = fetchMarkerDetails[block.NoCompactMark](ctx, bkt, noCompactMarkerFiles)


We should only load no-compact markers when they are displayed.

In store-gateway UI, I suggested to always display them, but in listblocks we could have provided an option instead.

pstibrany · 2023-12-11T09:16:10Z

tools/listblocks/main.go

@@ -140,7 +141,7 @@ func printMetas(metas map[ulid.ULID]*block.Meta, deletedTimes map[ulid.ULID]time
 	fmt.Fprintln(tabber)

 	for _, b := range blocks {
-		if !cfg.showDeleted && !deletedTimes[b.ULID].IsZero() {
+		if !cfg.showDeleted && deleteMarkerDetails[b.ULID].DeletionTime != 0 {


We could change DeletionTime to time.Time and provide correct marshal/unmarshal functions.

pstibrany · 2023-12-11T09:16:31Z

tools/listblocks/main.go

+		if val, ok := noCompactMarkerDetails[b.ULID]; ok {
+			fmt.Fprintf(tabber, "%v\t", []string{
+				fmt.Sprintf("Time: %s", time.Unix(val.NoCompactTime, 0).UTC().Format(time.RFC3339)),
+				fmt.Sprintf("Reason: %s", val.Reason)})
+		} else {
+			fmt.Fprintf(tabber, "\t")
+		}


What does the output for this look like?

ying-jeanne changed the title ~~Enhance: add no-compact filter on store-gateway/tenants endpoint~~ Enhance: Add no-compact filter on store-gateway/tenants endpoint Dec 7, 2023

pstibrany reviewed Dec 7, 2023

View reviewed changes

ying-jeanne force-pushed the mark-no-compact branch 5 times, most recently from c12106b to cf791b9 Compare December 7, 2023 18:33

ying-jeanne commented Dec 7, 2023

View reviewed changes

pkg/util/listblocks/listblocks.go Outdated Show resolved Hide resolved

ying-jeanne requested a review from pstibrany December 7, 2023 18:35

ying-jeanne changed the title ~~Enhance: Add no-compact filter on store-gateway/tenants endpoint~~ Store-gateway: Add no-compact filter on store-gateway/tenants endpoint Dec 7, 2023

ying-jeanne changed the title ~~Store-gateway: Add no-compact filter on store-gateway/tenants endpoint~~ Store-gateway: Add no-compact column on store-gateway/tenants admin UI Dec 7, 2023

ying-jeanne marked this pull request as ready for review December 8, 2023 09:38

ying-jeanne requested a review from a team as a code owner December 8, 2023 09:38

ying-jeanne requested a review from dimitarvdimitrov December 8, 2023 09:38

dimitarvdimitrov reviewed Dec 8, 2023

View reviewed changes

Enhance: add no-compact filter on store-gateway/tenants endpoint

e0f81fa

ying-jeanne force-pushed the mark-no-compact branch 4 times, most recently from b86c0b3 to 6a9ac39 Compare December 8, 2023 16:26

ying-jeanne requested a review from dimitarvdimitrov December 8, 2023 16:27

address comments

717060a

ying-jeanne force-pushed the mark-no-compact branch from 6a9ac39 to 717060a Compare December 8, 2023 16:33

dimitarvdimitrov reviewed Dec 8, 2023

View reviewed changes

pkg/storage/tsdb/block/markers.go Outdated Show resolved Hide resolved

pkg/storegateway/gateway_blocks_http.go Outdated Show resolved Hide resolved

pkg/storegateway/gateway_blocks_http.go Outdated Show resolved Hide resolved

fixes

38429a9

ying-jeanne force-pushed the mark-no-compact branch from 276aac9 to 38429a9 Compare December 8, 2023 17:13

ying-jeanne requested a review from dimitarvdimitrov December 8, 2023 17:13

dimitarvdimitrov approved these changes Dec 8, 2023

View reviewed changes

dimitarvdimitrov enabled auto-merge (squash) December 8, 2023 17:21

dimitarvdimitrov merged commit 6453122 into main Dec 8, 2023
28 checks passed

dimitarvdimitrov deleted the mark-no-compact branch December 8, 2023 17:26

pstibrany reviewed Dec 11, 2023

View reviewed changes

pstibrany mentioned this pull request Dec 11, 2023

Introduce type for Unix timestamps in JSON #6883

Merged

1 task

This was referenced Dec 13, 2023

Compactor: add function to remove no compact marker #6917

Merged

Store-gateway: allow mark/delete no-compaction from admin UI #6927

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store-gateway: Add no-compact column on store-gateway/tenants admin UI #6848

Store-gateway: Add no-compact column on store-gateway/tenants admin UI #6848

ying-jeanne commented Dec 7, 2023 •

edited

Loading

pstibrany Dec 7, 2023 •

edited

Loading

pstibrany Dec 7, 2023

dimitarvdimitrov Dec 8, 2023

ying-jeanne Dec 8, 2023

pstibrany left a comment

pstibrany Dec 11, 2023

pstibrany Dec 11, 2023

pstibrany Dec 11, 2023

pstibrany Dec 11, 2023

pstibrany Dec 11, 2023

pstibrany Dec 11, 2023

Store-gateway: Add no-compact column on store-gateway/tenants admin UI #6848

Store-gateway: Add no-compact column on store-gateway/tenants admin UI #6848

Conversation

ying-jeanne commented Dec 7, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

pstibrany Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ying-jeanne commented Dec 7, 2023 •

edited

Loading

pstibrany Dec 7, 2023 •

edited

Loading