Skip to content

Conversation

wujimin
Copy link
Contributor

@wujimin wujimin commented Aug 8, 2017

allowed depend servlet and vertx rest transport together, sdk will try to choose a better one.

wujimin added 5 commits August 8, 2017 15:55
if no listen address, then will not publish endpoint.

currently, all customers deployed microservice to web container, rest listen address is not null, so will not cause compatible problem.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 85.455% when pulling f1f9b22 on wujimin:auto-choose-same-name-transport into c52c246 on ServiceComb:master.


public static boolean canTcpListen(InetAddress address, int port) {
try (ServerSocket ss = new ServerSocket(port, 0, address)) {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

We should close the socket before return to avoid the resource leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is try final quick code, will auto invoke close

ss.close();

InetAddress address = InetAddress.getByName("127.0.0.1");
Assert.assertTrue(NetUtils.canTcpListen(address, port));
Copy link
Member

Choose a reason for hiding this comment

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

Please check if the port is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jdk make sure to call close

};

ServletRestTransport transport = new ServletRestTransport();
Assert.assertFalse(transport.canInit());
Copy link
Member

Choose a reason for hiding this comment

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

We may need to check the socket release here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same to above

setListenAddressWithoutSchema(TransportConfig.getAddress());

URIEndpointObject ep = (URIEndpointObject) getEndpoint().getAddress();
if (ep == null) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the meaning of null URIEndpointObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no listen address in microservice.yaml, that's mean will not register publish endpoint to SC.

};

VertxRestTransport transport = new VertxRestTransport();
Assert.assertTrue(transport.canInit());
Copy link
Member

Choose a reason for hiding this comment

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

Socket resource check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

released already.

Copy link
Member

@WillemJiang WillemJiang left a comment

Choose a reason for hiding this comment

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

OK, it sounds good.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 85.362% when pulling dd17867 on wujimin:auto-choose-same-name-transport into 553401a on ServiceComb:master.

buildTransportMap();

for (Transport transport : transportMap.values()) {
transportMap.put(transport.getName(), transport);
Copy link
Contributor

Choose a reason for hiding this comment

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

多了一条语句

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes
unused code, and will not cause problem
when i fetch and rebase, delete this line, then i can not push, git said that: rejected ..... (stale info)

i will delete this line in next PR.

@WillemJiang WillemJiang merged commit 0e177a6 into apache:master Aug 9, 2017
@wujimin wujimin deleted the auto-choose-same-name-transport branch August 23, 2017 08:54
@wujimin wujimin changed the title Auto choose same name transport JAV-282 Auto choose same name transport Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants