Skip to content
This repository has been archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-68] Make Wake unit tests robust to port conflicts #1068

Closed
wants to merge 1 commit into from
Closed

[REEF-68] Make Wake unit tests robust to port conflicts #1068

wants to merge 1 commit into from

Conversation

chenehk
Copy link
Contributor

@chenehk chenehk commented Jul 4, 2016

This addressed the issue by

  • Giving 0 for port numbers which leads to using random numbers as port numbers,
  • Getting port numbers with getListeningPort() for reuse.

JIRA:
REEF-68

@taegeonum
Copy link
Contributor

@EunhyangKim Thanks for the PR. I will take a look.

localAddressProvider, Tang.Factory.getTang().newInjector().getInstance(TcpPortProvider.class));

final RemoteIdentifierFactory factory = new DefaultRemoteIdentifierFactoryImplementation();
final RemoteIdentifier remoteId = factory.getNewInstance("socket://" + hostAddress + ":" + PORT);
final int port = rm.getListeningPort();
final RemoteIdentifier remoteId = factory.getNewInstance("socket://" + hostAddress + ":" + port);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use getMyIdentifier(): remoteId = rm.getMyIdentifier()

@taegeonum
Copy link
Contributor

taegeonum commented Jul 4, 2016

@EunhyangKim I did a pass. Could you please take a look at them and fix your code? Thanks!

This addressed the issue by
* Giving 0 for port numbers which leads to using random numbers as port numbers,
* Getting port numbers with getListeningPort() for reuse.
* Using getMyIdentifer() to directly get remote identifiers.

JIRA:
[REEF-68](https://issues.apache.org/jira/browse/REEF-68)
@chenehk
Copy link
Contributor Author

chenehk commented Jul 5, 2016

@taegeonum
Thank you for the review.
I have fixed what you mentioned in the comments. Please have a look. Thank you!

@taegeonum
Copy link
Contributor

Looks good to me! I'm merging it

@asfgit asfgit closed this in 684774b Jul 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants