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

zmq4: make number of retries for dial configurable #126

Merged
merged 2 commits into from Jun 20, 2022
Merged

zmq4: make number of retries for dial configurable #126

merged 2 commits into from Jun 20, 2022

Conversation

thielepaul
Copy link
Contributor

see #85

@codecov
Copy link

codecov bot commented Jun 13, 2022 •
edited

Codecov Report

Merging #126 (03b1695) into main (04c84de) will increase coverage by 0.54%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   67.63%   68.18%   +0.54%     
==========================================
  Files          29       29              
  Lines        1817     1820       +3     
==========================================
+ Hits         1229     1241      +12     
+ Misses        488      480       -8     
+ Partials      100       99       -1
Impacted Files Coverage Δ
options.go 83.33% <100.00%> (+23.33%) ⬆️
socket.go 83.90% <100.00%> (+0.67%) ⬆️
cxx_zmq4_compat.go 42.35% <0.00%> (-4.71%) ⬇️
rep.go 76.69% <0.00%> (+7.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04c84de...03b1695. Read the comment docs.

sbinet
sbinet reviewed Jun 13, 2022
View reviewed changes
socket.go Outdated Show resolved Hide resolved
options.go Show resolved Hide resolved
sbinet
sbinet reviewed Jun 13, 2022
View reviewed changes
Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

looking good.

thanks for the PR.

@thielepaul thielepaul force-pushed the configurable-number-of-retries branch 4 times, most recently from 3c0aab3 to e480f8f Compare June 15, 2022 19:31
@thielepaul
Copy link
Contributor Author

I just fixed the test

@sbinet
Copy link
Contributor

sbinet commented Jun 17, 2022

please rebase off the latest main, thanks.

@thielepaul thielepaul force-pushed the configurable-number-of-retries branch from e480f8f to f209f63 Compare June 17, 2022 09:08
@thielepaul
Copy link
Contributor Author

@sbinet the ipc-crouter-cdealer test always fails for me, and I cannot find the reason. Would it be ok for you if we skip the test for now, or do you have any idea, how my changes could be related to the test fail?

@sbinet
Copy link
Contributor

sbinet commented Jun 17, 2022

yeah, it's been flaky (the whole IPC transport actually).
I've disabled it:
#129

@thielepaul thielepaul force-pushed the configurable-number-of-retries branch from c8c471d to c33556f Compare June 17, 2022 11:37
@thielepaul
Copy link
Contributor Author

yeah, it's been flaky (the whole IPC transport actually). I've disabled it: #129

thanks, but this commit disables inproc-crouter-cdealer and not ipc-crouter-cdealer ;)

sbinet
sbinet reviewed Jun 17, 2022
View reviewed changes
socket_test.go Outdated Show resolved Hide resolved
socket_test.go Outdated
func TestConnMaxRetries(t *testing.T) {
retryCount := 123
socket := zmq4.NewSub(context.Background(), zmq4.WithDialerRetry(time.Microsecond), zmq4.WithDialerMaxRetries(retryCount))
testTransport.init()
Copy link
Contributor

Choose a reason for hiding this comment

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

sharing testTransport across tests feels like it may be a gun looking for a foot :)
(especially as we are relying on a specific value of dialCalledCount, shared across tests/transport)

perhaps register a specific transportMock value, together with a specific fake transport name, say "test-maxretries" (and "test-maxretries-infty" or something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good point, I gave each test a own transport

@thielepaul thielepaul force-pushed the configurable-number-of-retries branch from d75dc65 to ceebe76 Compare June 17, 2022 14:33
@thielepaul thielepaul force-pushed the configurable-number-of-retries branch from ceebe76 to 03b1695 Compare June 17, 2022 14:34
sbinet
sbinet approved these changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

LGTM

@sbinet sbinet merged commit ae18bc0 into go-zeromq:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

None yet
2 participants
@thielepaul @sbinet