Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ingester: shortcut fetchWants that have already been satisfied #9891

Conversation

dimitarvdimitrov
Copy link
Contributor

What this PR does

After #9855 and the assertions in pollFetchesAndAssertNoRecords there were some flaky tests (TestConcurrentFetchers/consume_from_end_and_update_immediately in particular). This happened because some fetch would satisfy the future fetchWants. Future fetchWantes would only end up buffering records and then discarding them at PollFetches.

The tests are more obviously flaky if you make consuming records a bit more expensive, for example, but changing the logger with

diff --git a/pkg/storage/ingest/fetcher_test.go b/pkg/storage/ingest/fetcher_test.go
index f96f27d5b4..b1d5df9aa9 100644
--- a/pkg/storage/ingest/fetcher_test.go
+++ b/pkg/storage/ingest/fetcher_test.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
 	"sync"
 	"testing"
 	"time"
@@ -1057,7 +1058,7 @@ func TestConcurrentFetchers(t *testing.T) {
 }
 
 func createConcurrentFetchers(ctx context.Context, t *testing.T, client *kgo.Client, topic string, partition int32, startOffset int64, concurrency, recordsPerFetch int) *concurrentFetchers {
-	logger := log.NewNopLogger()
+	logger := log.NewLogfmtLogger(os.Stdout)
 	reg := prometheus.NewPedanticRegistry()
 	metrics := newReaderMetrics(partition, reg, func() float64 { return 1 })

The fix

This PR shortcuts this, so the tests are less likely to be flaky. Now we can still make a request for a fetchWants that's been satisfied, but it's less likely and there will only be up to concurrency of these.

Checklist

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

After 9855 there were some flaky tests (`TestConcurrentFetchers/consume_from_end_and_update_immediately` in particular) because we'd buffer records and then discard them at PollFetches.

This PR shortcuts this, so the tests are less likely to be flaky. Now we can still make a request for a fetchWants that's been satisfied, but it's less likely and there will only be up to `concurrency` of these.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov
Copy link
Contributor Author

superseded by #9903

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant