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

Support for IPv6 #65

Merged
merged 3 commits into from
Apr 11, 2018
Merged

Support for IPv6 #65

merged 3 commits into from
Apr 11, 2018

Conversation

wallin
Copy link
Contributor

@wallin wallin commented Apr 6, 2018

Parse and connect to IPv6 hosts correctly

@codecov-io
Copy link

codecov-io commented Apr 6, 2018

Codecov Report

Merging #65 into master will increase coverage by 0.05%.
The diff coverage is 99.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage    94.9%   94.96%   +0.05%     
==========================================
  Files         143      143              
  Lines        8717     8810      +93     
==========================================
+ Hits         8273     8366      +93     
  Misses        444      444
Impacted Files Coverage Δ
lib/aerospike/host/parse.rb 94.73% <100%> (+2.42%) ⬆️
spec/aerospike/host/parse_spec.rb 100% <100%> (ø) ⬆️
lib/aerospike/socket/tcp.rb 100% <100%> (ø) ⬆️
spec/aerospike/peers/parse_spec.rb 100% <100%> (ø) ⬆️
spec/aerospike/socket/tcp_spec.rb 100% <100%> (ø) ⬆️
lib/aerospike/utils/string_parser.rb 100% <100%> (ø) ⬆️
lib/aerospike/peers/parse.rb 97.26% <95.45%> (-0.93%) ⬇️
lib/aerospike/socket/base.rb 85.18% <0%> (-1.86%) ⬇️
spec/aerospike/udf_spec.rb 100% <0%> (ø) ⬆️
lib/aerospike/cluster.rb 93.63% <0%> (+0.56%) ⬆️

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 e4047cb...8e89404. Read the comment docs.

@wallin
Copy link
Contributor Author

wallin commented Apr 6, 2018

@jhecking This remains to be tested in a real-world setup. Would you be able to test it quickly?

@jhecking
Copy link
Contributor

Looks good to me. I finally got IPv6 working on my Vagrant dev box today and was able to run some tests for IPv6-only as well as mixed setups.

I noticed only one minor issue with one of the existing specs while testing: The Client#connect spec checks whether the cluster size is >= 1 when using peers protocol as well as the older services protocol. In an IPv6-only setup I found that the older services protocol does not advertize IPv6 addresses. So when the peers protocol is disabled and only one seed node is given for a 2+-node cluster, only that seed node is discovered, but not the remaining nodes. The specs do not catch this. However, we can ignore this, since in a real-world setup, any cluster that supports IPv6 will use the peers protocol for cluster discovery.

@jhecking
Copy link
Contributor

@wallin, the PR is still tagged as "WIP" - is there any further work you plan to do on this?

@wallin wallin changed the title WIP: Support for IPv6 Support for IPv6 Apr 11, 2018
@wallin
Copy link
Contributor Author

wallin commented Apr 11, 2018

@jhecking

the PR is still tagged as "WIP" - is there any further work you plan to do on this?

nope. sorry about that. just forgot to change it.

So when the peers protocol is disabled and only one seed node is given for a 2+-node cluster, only that seed node is discovered, but not the remaining nodes.

Perhaps we could raise an error/warning if IPv6 is used when the peers protocol isn't supported? Happy to take a stab at it in a separate PR

@jhecking jhecking added this to the v2.7.0 milestone Apr 11, 2018
@jhecking jhecking self-requested a review April 11, 2018 03:25
Copy link
Contributor

@jhecking jhecking left a comment

Choose a reason for hiding this comment

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

👍

@jhecking
Copy link
Contributor

Perhaps we could raise an error/warning if IPv6 is used when the peers protocol isn't supported? Happy to take a stab at it in a separate PR

How would you know? If one of the seed addresses is IPv6? I think we can just ignore this use case since it only happens during testing. All servers that support IPv6 also support the peers protocol.

@jhecking jhecking merged commit 38c728c into aerospike:master Apr 11, 2018
@wallin
Copy link
Contributor Author

wallin commented Apr 11, 2018

How would you know? If one of the seed addresses is IPv6? I think we can just ignore this use case since it only happens during testing. All servers that support IPv6 also support the peers protocol.

Right, that was the idea. But ok let’s ignore this.

@jhecking
Copy link
Contributor

@wallin, I've tagged and released v2.7.0. Thanks again for your contributions to this release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants