Skip to content
New issue

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

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

Already on GitHub? # to your account

Make maxSleep and maxRetires configurable when building options #94

Merged
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

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

CLA assistant check
All committers have signed the CLA.

@kerthcet
Copy link
Contributor Author

kerthcet commented Feb 3, 2023

cc @prashantv for review maybe?

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
Signed-off-by: Kante Yin <kerthcet@gmail.com>
@kerthcet
Copy link
Contributor Author

kerthcet commented Feb 3, 2023

@sywhang PTAL

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
@codecov
Copy link

codecov bot commented Feb 12, 2023

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

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
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
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

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
@mway mway mentioned this pull request Oct 24, 2023
sywhang added a commit to sywhang/goleak that referenced this pull request Oct 24, 2023
sywhang added a commit that referenced this pull request Oct 24, 2023
…ns (#94)" (#116)

This reverts commit 751da59.

Per discussion in #93, we don't want to release this change as-is.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make maxRetries and maxSleep configurable in goleak
6 participants