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

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.
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.

Make maxRetries and maxSleep configurable in goleak
6 participants