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

Nonblocking socket initialisation #79

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

szymonsobczak
Copy link

This fixes #78.

@szymonsobczak szymonsobczak changed the title Nonblocking socket initialisation - solves issue #78 Nonblocking socket initialisation - solves #78 Dec 30, 2014
@szymonsobczak szymonsobczak changed the title Nonblocking socket initialisation - solves #78 Nonblocking socket initialisation - fixes #78 Dec 30, 2014
@szymonsobczak szymonsobczak changed the title Nonblocking socket initialisation - fixes #78 Nonblocking socket initialisation Dec 30, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.94%) when pulling 08c85fb on futuresimple:nonblocking_socket into b4dc2be on bpot:master.

@bpot
Copy link
Owner

bpot commented Dec 31, 2014

This looks great! I would like to have an integration test that reproduces the issue. Have you looked at writing one?

It would probably have to be linux only (b/c of iptables) but I think that's fine.

@szymonsobczak
Copy link
Author

Hi @bpot,

I've added the integration test. It requires

The spec fails when connection is using TCPSocket.new() (in my case it takes >30s to timeout) and passes with connect_with_timeout().

Some minor fixes were required to run the suite on OSX, like running OS commands differently and ensuring required dirs exist.

I've also worked a bit with sleeps, as the tests were unnecessarily slow and hard to work with. There is still room for improvement - most of the actions can be detected from watching kafka logs, so there should be no need for sleeps whatsoever.

BTW, we're using your library on production already for a couple months now and it looks stable, so maybe it's time to release v1.0.0?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.43%) to 91.51% when pulling b67a106 on futuresimple:nonblocking_socket into b4dc2be on bpot:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.84%) to 91.1% when pulling a6877d1 on futuresimple:nonblocking_socket into b4dc2be on bpot:master.

RSpec.describe "blocked port", :requires => :sudo_pfctl do
let(:topic) { "test" }
let(:cluster_metadata) { ClusterMetadata.new }
let(:message) { MessageToSend.new(topic, "dupa") }

Choose a reason for hiding this comment

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

@szymonsobczak hey, the content of MessageToSend could be better... ;)

@mensfeld
Copy link

is this going to be merged?

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.

Fetching metadata when no routing to leader does not comply to socket_timeout_ms parameter.
5 participants