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

Make maxSleep and maxRetires configurable when building options #94

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Make maxSleep and maxRetires configurable when building options #94

merged 3 commits into from
Feb 13, 2023

Conversation

kerthcet
Copy link
Contributor

@kerthcet kerthcet commented Feb 3, 2023

Make goleak options more flexible to adapt to users' variety scenarios

Signed-off-by: Kante Yin kerthcet@gmail.com

fix #93

@kerthcet
Make maxSleep and maxRetires configurable when building options
fbdd790 
Make goleak options more flexible to adapt to users' variety scenarios

Signed-off-by: Kante Yin <kerthcet@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Feb 3, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@kerthcet
Copy link
Contributor Author

kerthcet commented Feb 3, 2023

cc @prashantv for review maybe?

sywhang
sywhang reviewed Feb 3, 2023
View reviewed changes
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Left a few comments to improve the PR.

options.go Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
@kerthcet
Add export function comments
8b91807 
Signed-off-by: Kante Yin <kerthcet@gmail.com>
@kerthcet
Copy link
Contributor Author

kerthcet commented Feb 3, 2023

@sywhang PTAL

@pohly pohly mentioned this pull request Feb 6, 2023
Merged
pohly
pohly approved these changes Feb 6, 2023
View reviewed changes
Copy link

@pohly pohly left a comment

Choose a reason for hiding this comment

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

This looks good to me. It would be useful to have in a release soon together with the x/lint go.mod change because then I can simplify my Kubernetes PR: https://github.com/kubernetes/kubernetes/pull/115456/files#r1097270472

@kerthcet
Copy link
Contributor Author

kerthcet commented Feb 6, 2023

Thanks @pohly , this seems need the privilege of maintainer to run the workflow. cc @sywhang @prashantv

@pohly pohly mentioned this pull request Feb 12, 2023
Closed
@codecov
Copy link

codecov bot commented Feb 12, 2023 •
edited
Loading

Codecov Report

Merging #94 (717ca8e) into master (70e025e) will increase coverage by 1.59%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   94.92%   96.52%   +1.59%     
==========================================
  Files           5        5              
  Lines         138      230      +92     
==========================================
+ Hits          131      222      +91     
- Misses          4        5       +1     
  Partials        3        3
Impacted Files Coverage Δ
leaks.go 100.00% <100.00%> (ø)
options.go 100.00% <100.00%> (ø)
testmain.go 100.00% <0.00%> (ø)
tracestack_new.go 100.00% <0.00%> (ø)
internal/stack/stacks.go 88.73% <0.00%> (+3.31%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

abhinav
abhinav approved these changes Feb 13, 2023
View reviewed changes
Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

CC @sywhang

options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
@sywhang @abhinav
Apply suggestions from code review
717ca8e 
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
sywhang
sywhang approved these changes Feb 13, 2023
View reviewed changes
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution

@sywhang sywhang merged commit 751da59 into uber-go:master Feb 13, 2023
@prashantv
Copy link
Collaborator

prashantv commented Feb 13, 2023 •
edited
Loading

cc @prashantv for review maybe?

Sorry for the delay, as mentioned on the issue, I'm not sure we should be exposing these options, but try to capture a single option to configure retries (e.g., RetryTime). It seems like an implementation leak to expose the underlying num retries / max sleep etc.

@kerthcet kerthcet deleted the feat/add-maxRetries-and-maxSleep branch February 14, 2023 09:00
@abhinav abhinav mentioned this pull request Feb 17, 2023
Open
@mway mway mentioned this pull request Oct 24, 2023
Merged
sywhang added a commit to sywhang/goleak that referenced this pull request Oct 24, 2023
@sywhang
Revert "Make maxSleep and maxRetires configurable when building optio…
45771ac 
…ns (uber-go#94)"

This reverts commit 751da59.
@sywhang sywhang mentioned this pull request Oct 24, 2023
Merged
sywhang added a commit that referenced this pull request Oct 24, 2023
@sywhang
Revert "Make maxSleep and maxRetires configurable when building optio…
5643445 
…ns (#94)" (#116)

This reverts commit 751da59.

Per discussion in #93, we don't want to release this change as-is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sywhang sywhang sywhang approved these changes

@abhinav abhinav abhinav approved these changes

@pohly pohly pohly approved these changes

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

Successfully merging this pull request may close these issues.

Make maxRetries and maxSleep configurable in goleak
6 participants
@kerthcet @CLAassistant @prashantv @pohly @abhinav @sywhang