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.

Sign up for GitHub

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

Jump to bottom

fix: revert default listeners value and update docs #3314

Merged
merged 1 commit into from
Sep 10, 2019
Merged

fix: revert default listeners value and update docs #3314

merged 1 commit into from
Sep 10, 2019

Conversation

stevenpyzhang
Copy link
Member

@stevenpyzhang stevenpyzhang commented Sep 9, 2019 •
edited
Loading

Description

Follow up to: #3310
listeners=http://0.0.0.0:8088,http://[::]:8088 change in the previous PR will cause the server initialization to fail with:

...
[2019-09-09 12:49:35,916] INFO Starting server (io.confluent.ksql.rest.server.KsqlServerMain:75)
[2019-09-09 12:49:36,043] INFO Adding listener: http://0.0.0.0:8088 (io.confluent.rest.Application:241)
[2019-09-09 12:49:36,061] INFO Adding listener: http://[::]:8088 (io.confluent.rest.Application:241)
[2019-09-09 12:49:36,230] INFO jetty-9.4.18.v20190429; built: 2019-04-29T20:42:08.989Z; git: e1bc35120a6617ee3df052294e433f3a25ce7097; jvm 1.8.0_202-ea-b03 (org.eclipse.jetty.server.Server:370)
[2019-09-09 12:49:36,265] INFO DefaultSessionIdManager workerName=node0 (org.eclipse.jetty.server.session:365)
[2019-09-09 12:49:36,265] INFO No SessionScavenger set, using defaults (org.eclipse.jetty.server.session:370)
[2019-09-09 12:49:36,266] INFO node0 Scavenging every 660000ms (org.eclipse.jetty.server.session:149)
[2019-09-09 12:49:36,698] INFO HV000001: Hibernate Validator 5.1.3.Final (org.hibernate.validator.internal.util.Version:27)
[2019-09-09 12:49:36,814] INFO Started o.e.j.s.ServletContextHandler@592238c5{/,null,AVAILABLE} (org.eclipse.jetty.server.handler.ContextHandler:855)
[2019-09-09 12:49:36,843] INFO Started o.e.j.s.ServletContextHandler@1aa99005{/ws,null,AVAILABLE} (org.eclipse.jetty.server.handler.ContextHandler:855)
[2019-09-09 12:49:36,859] INFO Started NetworkTrafficServerConnector@377008df{HTTP/1.1,[http/1.1]}{0.0.0.0:8088} (org.eclipse.jetty.server.AbstractConnector:292)
[2019-09-09 12:49:36,859] INFO Server shutting down (io.confluent.ksql.rest.server.KsqlServerMain:80)
[2019-09-09 12:49:51,881] INFO Stopped NetworkTrafficServerConnector@377008df{HTTP/1.1,[http/1.1]}{0.0.0.0:8088} (org.eclipse.jetty.server.AbstractConnector:341)
[2019-09-09 12:49:51,881] INFO Stopped NetworkTrafficServerConnector@105b693d{HTTP/1.1,[http/1.1]}{[::]:8088} (org.eclipse.jetty.server.AbstractConnector:341)
[2019-09-09 12:49:51,882] INFO node0 Stopped scavenging (org.eclipse.jetty.server.session:167)
[2019-09-09 12:49:51,883] INFO Stopped o.e.j.s.ServletContextHandler@1aa99005{/ws,null,UNAVAILABLE} (org.eclipse.jetty.server.handler.ContextHandler:1045)
[2019-09-09 12:49:51,887] INFO Stopped o.e.j.s.ServletContextHandler@592238c5{/,null,UNAVAILABLE} (org.eclipse.jetty.server.handler.ContextHandler:1045)
[2019-09-09 12:49:51,888] ERROR Failed to start KSQL (io.confluent.ksql.rest.server.KsqlServerMain:64)
java.io.IOException: Failed to bind to /0:0:0:0:0:0:0:0:8088
	at org.eclipse.jetty.server.ServerConnector.openAcceptChannel(ServerConnector.java:346)
	at org.eclipse.jetty.server.ServerConnector.open(ServerConnector.java:308)
	at org.eclipse.jetty.server.AbstractNetworkConnector.doStart(AbstractNetworkConnector.java:80)
	at org.eclipse.jetty.server.ServerConnector.doStart(ServerConnector.java:236)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:68)
	at org.eclipse.jetty.server.Server.doStart(Server.java:396)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:68)
	at io.confluent.rest.Application.start(Application.java:746)
	at io.confluent.ksql.rest.server.KsqlRestApplication.start(KsqlRestApplication.java:205)
	at io.confluent.ksql.rest.server.KsqlServerMain.tryStartApp(KsqlServerMain.java:76)
	at io.confluent.ksql.rest.server.KsqlServerMain.main(KsqlServerMain.java:62)
Caused by: java.net.BindException: Address already in use
	at sun.nio.ch.Net.bind0(Native Method)
	at sun.nio.ch.Net.bind(Net.java:433)
	at sun.nio.ch.Net.bind(Net.java:425)
	at sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:223)
	at sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:74)
	at org.eclipse.jetty.server.ServerConnector.openAcceptChannel(ServerConnector.java:342)
	... 10 more

This doesn't happen when listeners=http://[::]:8088

[2019-09-09 12:49:36,043] INFO Adding listener: http://0.0.0.0:8088 (io.confluent.rest.Application:241)
[2019-09-09 12:49:36,061] INFO Adding listener: http://[::]:8088 (io.confluent.rest.Application:241)

indicates that both listeners are registered. http://[::]:8088 is considered the same as
http://0.0.0.0:8088

Testing done

./bin/ksql-server-start ./config/ksql-server.properties
with
listeners=http://[::]:8088

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@stevenpyzhang stevenpyzhang requested review from JimGalasyn and a team as code owners September 9, 2019 20:12
JimGalasyn
JimGalasyn approved these changes Sep 9, 2019
View reviewed changes
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

Good catch, thanks for the fix!

@stevenpyzhang stevenpyzhang changed the title fix:correct ipv6 address and update docs fix: correct ipv6 address and update docs Sep 9, 2019
spena
spena approved these changes Sep 9, 2019
View reviewed changes
Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

I don't have enough knowledge of IPv6, but if this change works for both IPv4 and IPv6 and it looks good to me.

@spena spena requested a review from a team September 9, 2019 20:27
rodesai
rodesai suggested changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

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

Loopback is a different interface. Have you tried using this config on an instance with an ipv6 address and testing whether you can talk to it?

@stevenpyzhang
Copy link
Member Author

@rodesai ,hmm you're right. The CLI can't communicate with just http://[::1]:8088. Do you have any idea of what the change would need to be then to distinguish the ipv4 from ipv6 so the initialization doesn't fail when they're both passed in? Or should this just be a docs change to indicate you can only specify one of http://[::]:8088 or http://0.0.0.0:8088?

@big-andy-coates
Copy link
Contributor

With a little bit more research it looks as though IPv6 handling may be some what platform dependant. Some platforms automatically route IPv4 to IPv6 listening ports and vice-versa. Others do not. On those that do, defining two listeners will result in a port clash, as the second listener is not needed. On those that don't, adding the second listener is often the way to go.

So... it looks like there is no one-size-fits-all for IPv6 handling.

I think the best we can do is have the default server.properties use http://0.0.0.0.0:8088 and have a comment in that file, and in the docs, that KSQL will listen on IPv6 if listeners is set to http://[::]:8088, and if you want to listen on both this will be platform dependant.

@stevenpyzhang stevenpyzhang changed the title fix: correct ipv6 address and update docs fix: revert default listeners value and update docs Sep 9, 2019
JimGalasyn
JimGalasyn approved these changes Sep 9, 2019
View reviewed changes
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenpyzhang
Copy link
Member Author

Related issue, #3319

agavra
agavra approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM - sync with @rodesai before shipping

@agavra agavra mentioned this pull request Sep 10, 2019
Closed
rodesai
rodesai approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenpyzhang stevenpyzhang merged commit 0ff4a51 into confluentinc:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimGalasyn JimGalasyn JimGalasyn approved these changes

@spena spena spena approved these changes

@agavra agavra agavra approved these changes

@rodesai rodesai rodesai approved these changes

@big-andy-coates big-andy-coates Awaiting requested review from big-andy-coates

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stevenpyzhang @big-andy-coates @rodesai @agavra @spena @JimGalasyn