Refactored forGivenIngestors to use DoUntilQuorum instead of Do #9984

akhilanarayanan · 2023-07-19T17:45:56Z

What this PR does / why we need it:
This changes the forGivenIngestors function in pkg/querier/ingester_querier.go to use DoUntilQuorum (from PR #293) instead of Do.

DoUntilQuorum is much more efficient than Do. The exact differences between them can be seen here.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

CLAassistant · 2023-07-19T17:46:01Z

All committers have signed the CLA.

JStickler

[docs team] I have questions.

JStickler · 2023-07-24T18:30:05Z

docs/sources/configure/_index.md

@@ -3624,7 +3624,19 @@ backoff_config:
  # CLI flag: -<prefix>.backoff-retries
  [max_retries: <int> | default = 10]

-# Enable TLS in the GRPC client. This flag needs to be enabled when any other
+# Initial stream window size. Values less than the default are not supported and
+# are ignored. Setting this to a value other than the default disables the BDP


Could we define BDP? There are 50 definitions on acronym finder.

Hiya! I'm really not sure what the changes in the _index.md file mean sorry. I was just failing the CI check-doc step and the console log told me to run make doc and commit the changes.

Ah gotcha. Now I have to go try to track down which PR added these changes.

@akhilanarayanan did you make any changes to this file? if you just gitc checkout this file, and also make sure you've merged the latest from main onto your branch, then the docs step on your PR shouldn't behave any differently than it does on main, and it's currently passing there.

JStickler · 2023-07-24T18:32:52Z

docs/sources/configure/_index.md

+# are ignored. Setting this to a value other than the default disables the BDP
+# estimator.
+# CLI flag: -<prefix>.initial-stream-window-size
+[initial_stream_window_size: <int> | default = 63KiB1023B]


Is this default value correct?

Huh, apparently it's correct? https://github.com/grafana/loki/pull/9920/files#r1261263633

trevorwhitney

Looks pretty good to me, just a few small things.

trevorwhitney · 2023-07-31T14:41:51Z

docs/sources/configure/_index.md

@@ -3624,7 +3624,19 @@ backoff_config:
  # CLI flag: -<prefix>.backoff-retries
  [max_retries: <int> | default = 10]

-# Enable TLS in the GRPC client. This flag needs to be enabled when any other
+# Initial stream window size. Values less than the default are not supported and
+# are ignored. Setting this to a value other than the default disables the BDP


@akhilanarayanan did you make any changes to this file? if you just gitc checkout this file, and also make sure you've merged the latest from main onto your branch, then the docs step on your PR shouldn't behave any differently than it does on main, and it's currently passing there.

trevorwhitney · 2023-07-31T14:43:59Z

pkg/storage/stores/series/series_index_store.go

@@ -279,7 +279,8 @@ func (c *indexReaderWriter) chunksToSeries(ctx context.Context, in []logproto.Ch
 		}))
 	}

-	results := make([]labels.Labels, 0, len(chunksBySeries))
+	//results := make([]labels.Labels, 0, len(chunksBySeries))


no need for this comment anymore

trevorwhitney · 2023-07-31T14:52:16Z

pkg/storage/stores/series/series_index_store.go

 			}
-			return err
+			resultsChan <- res


I am unable to recreate this bug locally (despite running with a count to 10k), but I'm curious if we just assign the results using results[idx] instead of introducing an additional concurrency data structure here, would that also fix the problem? I can see the race condition with reassigning results, but only assigning to specific indexes shouldn't have the same problem?

trevorwhitney

Looks great! Thanks @akhilanarayanan

bboreham · 2023-08-23T09:17:32Z

Do you think this could relate to #10320 ?

bboreham · 2023-08-23T09:21:02Z

I think I fixed one of the same bugs at #10310

bboreham

I think this has a better fix to the race condition than #10310.
I also think it should go in its own PR, with CHANGELOG entry, because it is fixing a serious bug and not part of the refactor.

bboreham · 2023-08-23T09:26:51Z

pkg/storage/stores/series/series_index_store.go

 			}
 			return err
 		},
 	); err != nil {
 		return nil, err
 	}

-	sort.Slice(results, func(i, j int) bool {
-		return labels.Compare(results[i], results[j]) < 0
+	flattened := make([]labels.Labels, 0) // Create a new slice to hold the flattened results


Why isn't this allocated to len(chunksBySeries) like before?

DylanGuedes

Congrats, this looks awesome!

A few suggestions:

Do you mind adding to the PR description the "why" of the move to "DoUntilQuorum"? Depending on it, this might be applicable in other places of the code base.
Changing the function used by ingesters is a delicate change, while most of the other code changes are not (ex: changing newTokens to be evaluated on a single line, or: to return responseFromIngesters instead of nil for when a error is found). For future occasions: If you're interested in getting your delicate changes merged/approved faster, I'd recommend isolating them, and not mixing with other changes. This way reviewers will need less time to feel confident approving/merging your change.
Other than that, gj! 🎉

@akhilanarayanan

Backport cf353bb from #10310 --- **What this PR does / why we need it**: ~Lock around data that is modified from multiple goroutines concurrently.~ Replaced with the implementation from #9984 since @akhilanarayanan did it more efficiently. **Which issue(s) this PR fixes**: Relates to #8586 **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - NA Documentation added - NA Tests updated - [x] `CHANGELOG.md` updated - NA If the change is worth mentioning in the release notes, add `add-to-release-notes` label - NA Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - NA For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) Co-authored-by: Bryan Boreham <[email protected]>

pull-request-size bot added the size/M label Jul 19, 2023

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jul 21, 2023

pull-request-size bot added size/L size/M and removed size/M size/L labels Jul 24, 2023

akhilanarayanan marked this pull request as ready for review July 24, 2023 17:34

akhilanarayanan requested review from JStickler and a team as code owners July 24, 2023 17:34

akhilanarayanan assigned akhilanarayanan and trevorwhitney Jul 24, 2023

JStickler reviewed Jul 24, 2023

View reviewed changes

trevorwhitney reviewed Jul 31, 2023

View reviewed changes

github-actions bot removed the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jul 31, 2023

akhilanarayanan unassigned trevorwhitney and akhilanarayanan Aug 17, 2023

akhilanarayanan and others added 9 commits August 21, 2023 12:08

Refactored forGivenIngestors to use DoUntilQuorum instead of Do

2b7935c

builds locally

7e1909c

fixed some linter stuff

908cf07

fixed a heisenbug that popped up in Test_GetSeries

0bccdcd

deleted unnecessary Do code

8480a81

undo go.mod changes

18a3874

got rid of channels and used indices to fix concurrency bug

1a449a0

added comment to concurrency thing

52f0871

fixed lint

a33f35f

trevorwhitney force-pushed the akhilanarayanan/replace-do-with-dountilquorum branch from 99d9419 to a33f35f Compare August 21, 2023 18:24

trevorwhitney approved these changes Aug 22, 2023

View reviewed changes

bboreham reviewed Aug 23, 2023

View reviewed changes

bboreham mentioned this pull request Aug 23, 2023

Series Index Store: fix race in GetSeries #10310

Merged

2 tasks

DylanGuedes reviewed Aug 23, 2023

View reviewed changes

trevorwhitney and others added 3 commits August 23, 2023 08:57

Merge branch 'main' into akhilanarayanan/replace-do-with-dountilquorum

9cddcfd

removed the concurrency fix because that should go in its own pr

10ed90d

got rid of unnecessary inline changes

d7f0db1

trevorwhitney merged commit 3303844 into main Aug 23, 2023
4 checks passed

trevorwhitney deleted the akhilanarayanan/replace-do-with-dountilquorum branch August 23, 2023 21:41

grafanabot mentioned this pull request Aug 25, 2023

[release-2.9.x] Series Index Store: fix race in GetSeries #10346

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored forGivenIngestors to use DoUntilQuorum instead of Do #9984

Refactored forGivenIngestors to use DoUntilQuorum instead of Do #9984

akhilanarayanan commented Jul 19, 2023 •

edited

Loading

CLAassistant commented Jul 19, 2023 •

edited

Loading

JStickler left a comment

JStickler Jul 24, 2023

akhilanarayanan Jul 24, 2023

JStickler Jul 25, 2023

trevorwhitney Jul 31, 2023

JStickler Jul 24, 2023

JStickler Jul 25, 2023

trevorwhitney left a comment

trevorwhitney Jul 31, 2023

trevorwhitney Jul 31, 2023

trevorwhitney Jul 31, 2023

trevorwhitney left a comment

bboreham commented Aug 23, 2023

bboreham commented Aug 23, 2023

bboreham left a comment

bboreham Aug 23, 2023

DylanGuedes left a comment •

edited

Loading

Refactored forGivenIngestors to use DoUntilQuorum instead of Do #9984

Refactored forGivenIngestors to use DoUntilQuorum instead of Do #9984

Conversation

akhilanarayanan commented Jul 19, 2023 • edited Loading

CLAassistant commented Jul 19, 2023 • edited Loading

JStickler 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

trevorwhitney 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

trevorwhitney left a comment

Choose a reason for hiding this comment

bboreham commented Aug 23, 2023

bboreham commented Aug 23, 2023

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DylanGuedes left a comment • edited Loading

Choose a reason for hiding this comment

akhilanarayanan commented Jul 19, 2023 •

edited

Loading

CLAassistant commented Jul 19, 2023 •

edited

Loading

DylanGuedes left a comment •

edited

Loading