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

STORM-2479: Fix port assignment race condition in storm-webapp tests #2075

Merged
merged 1 commit into from May 8, 2017

Conversation

srdo
Copy link
Contributor

@srdo srdo commented Apr 16, 2017

See https://issues.apache.org/jira/browse/STORM-2479

I think we should get rid of Utils.getAvailablePort entirely, but removing it from storm-webapp first seemed like a good opportunity to figure out how difficult it is.

@srdo srdo force-pushed the STORM-2479 branch 5 times, most recently from 8b79336 to f61f405 Compare April 17, 2017 01:52
@srdo
Copy link
Contributor Author

srdo commented Apr 17, 2017

Earlier test failure seems unrelated, was a windowing bolt test failure.

}

public void stop() {
if (_server != null)
_server.stop();
_server.stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

_server is still able to be null, so checking null would be safer.

(I'm assuming AuthUtils.GetTransportPlugin() can return null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, AuthUtils.GetTransportPlugin can't return null, and neither included implementation of ITransportPlugin can return null :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh great. Thanks for explaining. :)

}

/**
* @return true if ThriftServer is listening to requests?
*/
public boolean isServing() {
return _server != null && _server.isServing();
return _server.isServing();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@HeartSaVioR
Copy link
Contributor

Change looks good. Minor comments.

@HeartSaVioR
Copy link
Contributor

+1

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