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

!act #13490 Changed the callback to SocketOption to accept a channel instead of a Socket, this allows for using the nio features. #15568

Merged
merged 1 commit into from
Jul 24, 2014

Conversation

HawaiianSpork
Copy link
Contributor

Changed the callback to SocketOption to accept a channel instead of a Socket, this allows for using the nio features.

For example in Java 7 you can now join a multicast group:

case class JoinGroup(group: InetAddress, networkInterface: NetworkInterface) extends SocketOption {

  override def afterConnect(c: DatagramChannel): Unit = {
    c.join(group, networkInterface)
  }
}

  IO(Udp) ! Udp.Bind(self, new InetSocketAddress(MulticastListener.port),
    options=List(ReuseAddress(true),
      JoinGroup(MulticastListener.group, MulticastListener.interf)))

@akka-ci
Copy link

akka-ci commented Jul 19, 2014

Can one of the repo owners verify this patch?

@ktoso
Copy link
Member

ktoso commented Jul 19, 2014

Thanks for rebasing!

OK TO TEST

@2m
Copy link
Member

2m commented Jul 20, 2014

PLS BUILD

@akka-ci
Copy link

akka-ci commented Jul 20, 2014

Pull request validation: SUCCESS 👍
Refer to this link for build results: https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/382/

@2m
Copy link
Member

2m commented Jul 20, 2014

Thanks for the rebase. Could you squash to one commit?

@HawaiianSpork
Copy link
Contributor Author

Squashed.

@@ -13,19 +13,38 @@ object Inet {
*/
trait SocketOption {

def beforeDatagramBind(ds: DatagramSocket): Unit = ()
/**
* Action to be taken for this option before bind is called()
Copy link
Member

Choose a reason for hiding this comment

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

Parentheses are on the wrong word. Also same with next two methods.

@2m
Copy link
Member

2m commented Jul 21, 2014

LGTM, after minor nitpicks.

@2m
Copy link
Member

2m commented Jul 21, 2014

Since this is breaking SocketOption user API, I think we should mention changed method signatures in the migration guide.

@HawaiianSpork
Copy link
Contributor Author

Updated migration guide.

@ktoso
Copy link
Member

ktoso commented Jul 22, 2014

LGTM

1 similar comment
@drewhk
Copy link
Member

drewhk commented Jul 22, 2014

LGTM

@2m
Copy link
Member

2m commented Jul 22, 2014

I just noticed that commit message mentions issue number from assembla. Could you change it to #13490 and then we can merge.

…nel instead of a Socket, this allows for using the nio features.

For example in Java 7 you can now join a multicast group:

case class JoinGroup(group: InetAddress, networkInterface: NetworkInterface) extends SocketOption {

  override def afterConnect(c: DatagramChannel): Unit = {
    c.join(group, networkInterface)
  }
}

  IO(Udp) ! Udp.Bind(self, new InetSocketAddress(MulticastListener.port),
    options=List(ReuseAddress(true),
      JoinGroup(MulticastListener.group, MulticastListener.interf)))

Other minor changes:

 - changed all methods in SocketOption to take a Channel instead of a Socket.  The socket can be gotten from the Channel but not the reverse.
 - all methods that are called before the bind are now called beforeBind for consistency.
 - All network connections now call the beforeBind and afterConnect.
@HawaiianSpork HawaiianSpork changed the title !act #3490 Changed the callback to SocketOption to accept a channel instead of a Socket, this allows for using the nio features. !act #13490 Changed the callback to SocketOption to accept a channel instead of a Socket, this allows for using the nio features. Jul 23, 2014
2m added a commit that referenced this pull request Jul 24, 2014
!act #13490 Changed the callback to SocketOption to accept a channel instead of a Socket, this allows for using the nio features.
@2m 2m merged commit bb56b88 into akka:master Jul 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants