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.

Sign up for GitHub

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

Jump to bottom

Go1.23 timers #11334

Closed
wants to merge 5 commits into from
Closed

Go1.23 timers #11334

wants to merge 5 commits into from

Conversation

shivanthzen
Copy link

@shivanthzen shivanthzen commented Oct 2, 2024 •
edited
Loading

Link to tracking issue

Fixes #11332

Testing

Documentation

@shivanthzen shivanthzen requested a review from a team as a code owner October 2, 2024 21:48
@shivanthzen shivanthzen requested a review from codeboten October 2, 2024 21:48
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Oct 2, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov Codecov
Copy link

codecov bot commented Oct 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.51%. Comparing base (4ace638) to head (05ac9dc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
processor/batchprocessor/batch_processor_1_22.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #11334   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files         430      429    -1     
  Lines       20226    20158   -68     
=======================================
- Hits        18509    18447   -62     
+ Misses       1342     1337    -5     
+ Partials      375      374    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codeboten
codeboten reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shivanthzen, one question

processor/batchprocessor/batch_processor_1_23.go
package batchprocessor // import "go.opentelemetry.io/collector/processor/batchprocessor"

func (b *shard) stopTimer() {
if b.hasTimer() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test to capture the regression this is fixing?

@bogdandrutu @codeboten
Update processor/batchprocessor/batch_processor.go
43ccc5e 
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
mx-psi
mx-psi reviewed Oct 3, 2024
View reviewed changes
processor/batchprocessor/batch_processor_1_22.go Outdated
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

//go:build !1.23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be go1.23? At least on https://stackoverflow.com/a/38439941 that is the format used. Has the format changed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For builds using go version 1.23 and above , file tagged with //go:build 1.23 will be used, for go version below 1.23 , file tagged with //go:build !1.23 will be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, what I meant is that I expected this to be

Suggested change
//go:build !1.23
//go:build !go1.23

or at least this is how it used to work for Go in older versions, not 1.23

@shivanthzen
Copy link
Author

I suggest this MR closed without merging in favour of golang/go#69312.

@codeboten
Copy link
Contributor

It looks like the fix went into 1.23.2 https://github.com/golang/go/issues?q=milestone%3AGo1.23.2+label%3ACherryPickApproved, golang/go#69333

Is the fix for us to ensure we use 1.23.2 when building the next release?

@mx-psi
Copy link
Member

mx-psi commented Oct 3, 2024

Double checked by re-running a random test in the -releases repository for the last commit:
https://github.com/open-telemetry/opentelemetry-collector-releases/actions/runs/11152590145/job/31035238607

Looks like we are all set:

Setup go version spec ~1.23
Attempting to download ~1.23...
matching ~1.23...
Acquiring 1.23.2 from https://github.com/actions/go-versions/releases/download/1.23.2-11145[9](https://github.com/open-telemetry/opentelemetry-collector-releases/actions/runs/11152590145/job/31035238607#step:4:10)22912/go-1.23.2-linux-x64.tar.gz

@codeboten codeboten mentioned this pull request Oct 3, 2024
Closed
@codeboten
Copy link
Contributor

Might be worth at least capturing the fix in a changelog if no code changes are required

@shivanthzen
Copy link
Author

Parent issue has been closed. Fix was implemented in golang

@shivanthzen shivanthzen closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mx-psi mx-psi mx-psi left review comments

@codeboten codeboten codeboten left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Batch processor can deadlock/freeze using Go 1.23 timers
4 participants
@shivanthzen @codeboten @mx-psi @bogdandrutu