[improve][broker] Add parameter check for create/update cluster. #19151

mattisonchao · 2023-01-06T09:27:27Z

Motivation

In the current implementation, we didn't have any cluster data check for create/update cluster. it may cause some problems when the user sets the wrong URL.

Modifications

Add parameters check.

Verifying this change

Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

The REST endpoints

Documentation

doc-not-needed

mattisonchao · 2023-01-06T09:29:37Z

@codelipenghui @eolivelli @Jason918 @congbobo184 @nicoloboschi
Would you guys mind taking a look?

codecov-commenter · 2023-01-06T12:04:28Z

Codecov Report

Merging #19151 (3e63d54) into master (492a9c3) will decrease coverage by 3.16%.
The diff coverage is 47.82%.

@@             Coverage Diff              @@
##             master   #19151      +/-   ##
============================================
- Coverage     47.46%   44.29%   -3.17%     
+ Complexity    10727    10057     -670     
============================================
  Files           711      713       +2     
  Lines         69456    69683     +227     
  Branches       7452     7483      +31     
============================================
- Hits          32964    30865    -2099     
- Misses        32810    35206    +2396     
+ Partials       3682     3612      -70

Flag	Coverage Δ
unittests	`44.29% <47.82%> (-3.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java	`15.06% <0.00%> (-0.04%)`	⬇️
.../apache/pulsar/broker/admin/impl/ClustersBase.java	`76.68% <81.81%> (+0.02%)`	⬆️
...lsar/client/impl/conf/ClientConfigurationData.java	`96.66% <100.00%> (ø)`
...pache/pulsar/client/impl/CompactionReaderImpl.java	`0.00% <0.00%> (-90.00%)`	⬇️
...g/apache/pulsar/client/impl/RawBatchConverter.java	`3.12% <0.00%> (-89.07%)`	⬇️
.../pulsar/compaction/StrategicTwoPhaseCompactor.java	`0.00% <0.00%> (-76.20%)`	⬇️
...sar/broker/stats/metrics/ManagedLedgerMetrics.java	`23.88% <0.00%> (-76.12%)`	⬇️
...lsar/client/impl/RawBatchMessageContainerImpl.java	`0.00% <0.00%> (-73.14%)`	⬇️
...ice/streamingdispatch/PendingReadEntryRequest.java	`0.00% <0.00%> (-68.19%)`	⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java	`3.70% <0.00%> (-64.82%)`	⬇️
... and 102 more

pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/ClusterDataTest.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LinuxInfoUtils.java

…ce/LinuxInfoUtils.java

congbobo184 · 2023-01-09T02:51:55Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminTest.java

@@ -775,7 +773,7 @@ public void resourceQuotas() throws Exception {
        TenantInfoImpl admin = TenantInfoImpl.builder()
                .allowedClusters(Collections.singleton(cluster))
                .build();
-        ClusterDataImpl clusterData = ClusterDataImpl.builder().serviceUrl(cluster).build();
+        ClusterDataImpl clusterData = ClusterDataImpl.builder().serviceUrl("http://example.pulsar").build();


Is this a breaking change? I haven't reviewed the details.

It's not a breaking change; we just didn't have data validation.

Technoboy-

LGTM

lhotari · 2023-01-12T19:50:27Z

@mattisonchao Is this PR a potential breaking change? Please check #19212 .
("Pulsar CI Flaky" jobs have been failing for several days with OOME after this PR was merged: https://github.com/apache/pulsar/actions/workflows/pulsar-ci-flaky.yaml . The OOME was a side-effect of the test class that broke because of the validation change.)

lhotari · 2023-01-12T19:51:16Z

@mattisonchao I wonder if a "https" url would need to be accepted in the serviceUrl just because of backwards compatibility reasons?

michaeljmarshall · 2023-01-12T19:53:05Z

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java

+    public void checkPropertiesIfPresent() throws IllegalArgumentException {
+        URIPreconditions.checkURIIfPresent(getServiceUrl(),
+                uri -> Objects.equals(uri.getScheme(), "http"),
+                "Illegal service url, example: http://pulsar.example.com:8080");
+        URIPreconditions.checkURIIfPresent(getServiceUrlTls(),
+                uri -> Objects.equals(uri.getScheme(), "https"),
+                "Illegal service tls url, example: https://pulsar.example.com:8443");
+        URIPreconditions.checkURIIfPresent(getBrokerServiceUrl(),
+                uri -> Objects.equals(uri.getScheme(), "pulsar"),
+                "Illegal broker service url, example: pulsar://pulsar.example.com:6650");
+        URIPreconditions.checkURIIfPresent(getBrokerServiceUrlTls(),
+                uri -> Objects.equals(uri.getScheme(), "pulsar+ssl"),
+                "Illegal broker service tls url, example: pulsar+ssl://pulsar.example.com:6651");
+        URIPreconditions.checkURIIfPresent(getProxyServiceUrl(),
+                uri -> Objects.equals(uri.getScheme(), "pulsar")
+                        || Objects.equals(uri.getScheme(), "pulsar+ssl"),
+                "Illegal proxy service url, example: pulsar+ssl://ats-proxy.example.com:4443 "
+                        + "or pulsar://ats-proxy.example.com:4080");
+    }


Do we document these requirements? Also, what would happen if a user had already created a cluster in a bad state?

[improve][broker] Add parameter check for cluster data.

dc45c82

mattisonchao self-assigned this Jan 6, 2023

mattisonchao added the area/broker label Jan 6, 2023

mattisonchao requested review from codelipenghui, congbobo184, Jason918, Technoboy-, eolivelli and nicoloboschi January 6, 2023 09:28

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 6, 2023

mattisonchao added this to the 2.12.0 milestone Jan 6, 2023

mattisonchao added the ready-to-test label Jan 6, 2023

mattisonchao closed this Jan 6, 2023

mattisonchao reopened this Jan 6, 2023

mattisonchao added 2 commits January 6, 2023 19:38

Fix checkstyle

7ca2dee

Fix error

fe5c8a1

mattisonchao added 4 commits January 6, 2023 22:05

Fix conflict

06782d8

Fix test

ad173d5

Fix failed test

a0846d8

Continue fix illgeal usage

d892a74

mattisonchao added release/important-notice The changes which are important should be mentioned in the release note and removed release/important-notice The changes which are important should be mentioned in the release note labels Jan 8, 2023

gaoran10 reviewed Jan 8, 2023

View reviewed changes

pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/ClusterDataTest.java Show resolved Hide resolved

mattisonchao commented Jan 9, 2023

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LinuxInfoUtils.java Outdated Show resolved Hide resolved

Update pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalan…

3e63d54

…ce/LinuxInfoUtils.java

congbobo184 reviewed Jan 9, 2023

View reviewed changes

gaoran10 approved these changes Jan 9, 2023

View reviewed changes

Technoboy- approved these changes Jan 9, 2023

View reviewed changes

coderzc approved these changes Jan 9, 2023

View reviewed changes

congbobo184 approved these changes Jan 9, 2023

View reviewed changes

mattisonchao merged commit e829672 into master Jan 9, 2023

mattisonchao deleted the paramter_check branch January 9, 2023 12:44

lhotari mentioned this pull request Jan 12, 2023

[fix][ci] Fix PulsarFunctionLocalRunTest that broke after ClusterData validation changes #19212

Merged

4 tasks

michaeljmarshall reviewed Jan 12, 2023

View reviewed changes

nodece mentioned this pull request Mar 9, 2023

[fix][broker] Fix create cluster with empty url #19762

Merged

5 tasks

michaeljmarshall mentioned this pull request Mar 20, 2023

Verify at least one URL is present for cluster creation #19866

Closed

2 tasks

sijie mentioned this pull request Mar 20, 2023

ISSUE-19866: Verify at least one URL is present for cluster creation streamnative/pulsar-archived#5638

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] Add parameter check for create/update cluster. #19151

[improve][broker] Add parameter check for create/update cluster. #19151

mattisonchao commented Jan 6, 2023

mattisonchao commented Jan 6, 2023

codecov-commenter commented Jan 6, 2023 •

edited

Loading

congbobo184 Jan 9, 2023

mattisonchao Jan 9, 2023

Technoboy- left a comment

lhotari commented Jan 12, 2023

lhotari commented Jan 12, 2023

michaeljmarshall Jan 12, 2023

[improve][broker] Add parameter check for create/update cluster. #19151

[improve][broker] Add parameter check for create/update cluster. #19151

Conversation

mattisonchao commented Jan 6, 2023

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

mattisonchao commented Jan 6, 2023

codecov-commenter commented Jan 6, 2023 • edited Loading

Codecov Report

congbobo184 Jan 9, 2023

Choose a reason for hiding this comment

mattisonchao Jan 9, 2023

Choose a reason for hiding this comment

Technoboy- left a comment

Choose a reason for hiding this comment

lhotari commented Jan 12, 2023

lhotari commented Jan 12, 2023

michaeljmarshall Jan 12, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jan 6, 2023 •

edited

Loading