Skip to content

Make maxSleep and maxRetires configurable when building options#94

Merged
sywhang merged 3 commits intouber-go:masterfrom
kerthcet:feat/add-maxRetries-and-maxSleep
Feb 13, 2023
Merged

Make maxSleep and maxRetires configurable when building options#94
sywhang merged 3 commits intouber-go:masterfrom
kerthcet:feat/add-maxRetries-and-maxSleep

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.

@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

@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

@abhinav abhinav abhinav approved these changes

@sywhang sywhang sywhang approved these changes

+1 more reviewer

@pohly pohly pohly approved these changes

Reviewers whose approvals may not affect merge requirements

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