From 4bbb60402f262214aecacb24839e75159143a43f Mon Sep 17 00:00:00 2001 From: Dmitrii Anoshin Date: Thu, 30 May 2024 12:18:33 -0700 Subject: [PATCH 1/2] [chore] Remove redundant wait group from a test (#10269) https://github.com/open-telemetry/opentelemetry-collector/pull/10258/files#r1621017272 made me wonder if the wait group is really necessary. We have another synchronization that waits for at least two requests to enter the merge function, which should be enough. So, we don't necessarily need this wait group. Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com> --- exporter/exporterhelper/batch_sender_test.go | 26 ++++++-------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/exporter/exporterhelper/batch_sender_test.go b/exporter/exporterhelper/batch_sender_test.go index 08ab42f89b5..bc59c0d63c5 100644 --- a/exporter/exporterhelper/batch_sender_test.go +++ b/exporter/exporterhelper/batch_sender_test.go @@ -443,9 +443,10 @@ func TestBatchSender_ShutdownDeadlock(t *testing.T) { waitMerge := make(chan struct{}, 10) // blockedBatchMergeFunc blocks until the blockMerge channel is closed - blockedBatchMergeFunc := func(_ context.Context, r1 Request, _ Request) (Request, error) { + blockedBatchMergeFunc := func(_ context.Context, r1 Request, r2 Request) (Request, error) { waitMerge <- struct{}{} <-blockMerge + r1.(*fakeRequest).items += r2.(*fakeRequest).items return r1, nil } @@ -458,18 +459,11 @@ func TestBatchSender_ShutdownDeadlock(t *testing.T) { sink := newFakeRequestSink() - // Send 10 concurrent requests and wait for them to start - startWG := sync.WaitGroup{} - for i := 0; i < 10; i++ { - startWG.Add(1) - go func() { - startWG.Done() - require.NoError(t, be.send(context.Background(), &fakeRequest{items: 4, sink: sink})) - }() - } - startWG.Wait() + // Send 2 concurrent requests + go func() { require.NoError(t, be.send(context.Background(), &fakeRequest{items: 4, sink: sink})) }() + go func() { require.NoError(t, be.send(context.Background(), &fakeRequest{items: 4, sink: sink})) }() - // Wait for at least one batch to enter the merge function + // Wait for the requests to enter the merge function <-waitMerge // Initiate the exporter shutdown, unblock the batch merge function to catch possible deadlocks, @@ -485,12 +479,8 @@ func TestBatchSender_ShutdownDeadlock(t *testing.T) { close(blockMerge) <-doneShutdown - // The exporter should have sent only one "merged" batch, in some cases it might send two if the shutdown - // happens before the batch is fully merged. - assert.LessOrEqual(t, uint64(1), sink.requestsCount.Load()) - - // blockedBatchMergeFunc just returns the first request, so the items count should be 4 times the requests count. - assert.Equal(t, sink.requestsCount.Load()*4, sink.itemsCount.Load()) + assert.EqualValues(t, 1, sink.requestsCount.Load()) + assert.EqualValues(t, 8, sink.itemsCount.Load()) } func queueBatchExporter(t *testing.T, batchOption Option) *baseExporter { From d55d04226e578a2ae1fb2fae6a44fb9f214bcc07 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Fri, 31 May 2024 10:28:50 -0700 Subject: [PATCH 2/2] [chore] remove TODO from confmap code (#10278) Remove a TODO comment on confmap/provider.go related to ChangeEvent. This struct is used extensively in providers that rely on changes such as: https://github.com/signalfx/splunk-otel-collector/blob/main/internal/configsource/etcd2configsource/source.go#L84 --- confmap/provider.go | 1 - 1 file changed, 1 deletion(-) diff --git a/confmap/provider.go b/confmap/provider.go index 95c0581327e..192577ed4d8 100644 --- a/confmap/provider.go +++ b/confmap/provider.go @@ -89,7 +89,6 @@ type Provider interface { type WatcherFunc func(*ChangeEvent) // ChangeEvent describes the particular change event that happened with the config. -// TODO: see if this can be eliminated. type ChangeEvent struct { // Error is nil if the config is changed and needs to be re-fetched. // Any non-nil error indicates that there was a problem with watching the config changes.