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 init WebSocketService with ClusterData #11234

Merged
merged 2 commits into from Jul 6, 2021
Merged

Fix init WebSocketService with ClusterData #11234

merged 2 commits into from Jul 6, 2021

Conversation

Technoboy-
Copy link
Contributor

Fixes #11233 .

Motivation

  1. Fix init WebSocketService with ClusterData.(before is null).
  2. As ProxyServiceStarter is using in script or command line, no test case is covered. So it's better to add test case . But this class inits from config file, it's difficult to set the port or config due to the dynamic generation of port in the test case. So I adjusted the implementation of the class.
  3. Add test case for ProxyServiceStarter.

@Technoboy-
1. Fix init WebSocketService with ClusterData.
ad10d2b 
2. Refactor ProxyServiceStarter to be easier for test.
3. Add test case for ProxyServiceStarter.
@codelipenghui codelipenghui added this to the 2.9.0 milestone Jul 6, 2021
@codelipenghui codelipenghui added release/2.8.1 release/2.7.3 type/bug The PR fixed a bug or issue reported a bug labels Jul 6, 2021
@Technoboy-
Copy link
Contributor Author

/pulsarbot run-failure-checks

codelipenghui
codelipenghui reviewed Jul 6, 2021
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Great work!

The change LGTM, just left some minor comments about the tests

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java
Comment on lines +58 to +61
serviceStarter.getConfig().setBrokerServiceURL(pulsar.getBrokerServiceUrl());
serviceStarter.getConfig().setBrokerWebServiceURL(pulsar.getWebServiceAddress());
serviceStarter.getConfig().setServicePort(Optional.of(11000));
serviceStarter.getConfig().setWebSocketServiceEnabled(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we are setting the configurations through the Java API, do we need to load the proxy.conf file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we are setting the configurations through the Java API, do we need to load the proxy.conf file?

ProxyServiceStarter load the config from command line "-c" and do this in the setup method.
But our test cases that start broker service listening on port 0, which would listen a port randomly.
We can't specified the brokerServerUrl in the proxy.conf then. for we dont know the port in actual. So I adjust the implemetation of ProxyServiceStarter, let it expose ProxyConfiguration, and we can set configs to runtime ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the explanation

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java Outdated Show resolved Hide resolved
codelipenghui
codelipenghui approved these changes Jul 6, 2021
View reviewed changes
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java
Comment on lines +58 to +61
serviceStarter.getConfig().setBrokerServiceURL(pulsar.getBrokerServiceUrl());
serviceStarter.getConfig().setBrokerWebServiceURL(pulsar.getWebServiceAddress());
serviceStarter.getConfig().setServicePort(Optional.of(11000));
serviceStarter.getConfig().setWebSocketServiceEnabled(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the explanation

@codelipenghui codelipenghui merged commit 93e145b into apache:master Jul 6, 2021
codelipenghui pushed a commit that referenced this pull request Jul 7, 2021
@Technoboy- @codelipenghui
Fix init WebSocketService with ClusterData (#11234)
aa290ae 
1. Fix init WebSocketService with ClusterData.
2. Refactor ProxyServiceStarter to be easier for test.
3. Add test case for ProxyServiceStarter.

(cherry picked from commit 93e145b)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 7, 2021
codelipenghui pushed a commit that referenced this pull request Jul 7, 2021
@Technoboy- @codelipenghui
Fix init WebSocketService with ClusterData (#11234)
208ca5c 
1. Fix init WebSocketService with ClusterData.
2. Refactor ProxyServiceStarter to be easier for test.
3. Add test case for ProxyServiceStarter.

(cherry picked from commit 93e145b)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jul 7, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@Technoboy- @ciaocloud
Fix init WebSocketService with ClusterData (apache#11234)
ce8ebc9 
1. Fix init WebSocketService with ClusterData.
2. Refactor ProxyServiceStarter to be easier for test.
3. Add test case for ProxyServiceStarter.
@Technoboy- Technoboy- deleted the fix-proxyServiceStarter branch August 10, 2022 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@gaoran10 gaoran10 Awaiting requested review from gaoran10

Assignees

@Technoboy- Technoboy-

Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.7.3 release/2.8.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.

Failed to retrieve Cluster data due to empty ConfigurationStoreServers
3 participants
@Technoboy- @eolivelli @codelipenghui