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

Allow to share multicast socket within jvm #5410

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@kimchy
Member

kimchy commented Mar 12, 2014

Due to bugs in jvm (specifically OSX), running zen discovery tests causes for "socket close" failure on receive on multicast socket, and under some jvm versions, even crashes. This happens because of the creation of multiple multicast sockets within the same VM. In practice, in our tests, we use the same settings, so we can share the same multicast socket across multiple channels.
This change creates an abstraction called MulticastChannel, that can be shared, with ref counting. Today, the shared option is only enabled under OSX.

Allow to share multicast socket within jvm
Due to bugs in jvm (specifically OSX), running zen discovery tests causes for "socket close" failure on receive on multicast socket, and under some jvm versions, even crashes. This happens because of the creation of multiple multicast sockets within the same VM. In practice, in our tests, we use the same settings, so we can share the same multicast socket across multiple channels.
This change creates an abstraction called MulticastChannel, that can be shared, with ref counting. Today, the shared option is only enabled under OSX.
/**
* Close the channel.
*/
public void close() {

This comment has been minimized.

@s1monw

s1monw Mar 13, 2014

Contributor

can this impl. Closeable?

@s1monw

s1monw Mar 13, 2014

Contributor

can this impl. Closeable?

This comment has been minimized.

@s1monw

s1monw Mar 13, 2014

Contributor

and also is it ok to call this more than once or should we protect it?

@s1monw

s1monw Mar 13, 2014

Contributor

and also is it ok to call this more than once or should we protect it?

This comment has been minimized.

@kimchy

kimchy Mar 13, 2014

Member

yea, will add Closeable, I think we should protect, in the subclasses we do, might make sense to bring it here....

@kimchy

kimchy Mar 13, 2014

Member

yea, will add Closeable, I think we should protect, in the subclasses we do, might make sense to bring it here....

multicastSocket = buildMulticastSocket(config);
} else {
logger.warn("failed to receive packet, throttling...", e);
Thread.sleep(500);

This comment has been minimized.

@s1monw

s1monw Mar 13, 2014

Contributor

500ms seems highish?

@s1monw

s1monw Mar 13, 2014

Contributor

500ms seems highish?

This comment has been minimized.

@kimchy

kimchy Mar 13, 2014

Member

this is what we had before in the zen discovery (refactored out the code), its actually pretty low sadly...

@kimchy

kimchy Mar 13, 2014

Member

this is what we had before in the zen discovery (refactored out the code), its actually pretty low sadly...

}
@Override
protected void close(Listener listener) {

This comment has been minimized.

@s1monw

s1monw Mar 13, 2014

Contributor

I think we can clean this up a bit with a AtomicBoolean isClosed that protects double closing. that way we don't need to make multicastSocket volatile and can make it final as well, makes sense?

@s1monw

s1monw Mar 13, 2014

Contributor

I think we can clean this up a bit with a AtomicBoolean isClosed that protects double closing. that way we don't need to make multicastSocket volatile and can make it final as well, makes sense?

This comment has been minimized.

@kimchy

kimchy Mar 13, 2014

Member

agreed, I added an AtomicBoolean in the base class

@kimchy

kimchy Mar 13, 2014

Member

agreed, I added an AtomicBoolean in the base class

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Mar 13, 2014

Member

applied first review comments

Member

kimchy commented Mar 13, 2014

applied first review comments

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Mar 14, 2014

Contributor

two minor comments but other than that LGTM

Contributor

s1monw commented Mar 14, 2014

two minor comments but other than that LGTM

@kimchy kimchy closed this in 3755f8e Mar 14, 2014

kimchy added a commit that referenced this pull request Mar 14, 2014

Allow to share multicast socket within jvm
Due to bugs in jvm (specifically OSX), running zen discovery tests causes for "socket close" failure on receive on multicast socket, and under some jvm versions, even crashes. This happens because of the creation of multiple multicast sockets within the same VM. In practice, in our tests, we use the same settings, so we can share the same multicast socket across multiple channels.
This change creates an abstraction called MulticastChannel, that can be shared, with ref counting. Today, the shared option is only enabled under OSX.
closes #5410

@kimchy kimchy deleted the kimchy:shared_multicast branch Mar 14, 2014

kimchy added a commit that referenced this pull request Mar 14, 2014

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