[improve] [broker] Verify at least one URL is present for cluster creation #20821

harshithasudhakar · 2023-07-17T08:46:21Z

Main/Original Issue: apache#19866

Motivation

Modifications

Checking if serviceUrl, serviceUrlTls, brokerServiceUrl, or brokerServiceUrlTls are not null. Then proceeds to throw IllegalArgumentException if either of serviceUrl, serviceUrlTls or brokerServiceUrl, brokerServiceUrlTls has not been set.

Verifying this change

Make sure that the change passes the CI checks.

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

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Checking if `serviceUrl`, `serviceUrlTls`, `brokerServiceUrl`, or `brokerServiceUrlTls` are not null. Then proceeds to throw `IllegalArgumentException` if either of `serviceUrl`, `serviceUrlTls` or `brokerServiceUrl`, `brokerServiceUrlTls` has not been set.

github-actions · 2023-07-17T08:46:54Z

@harshithasudhakar Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

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

Technoboy-

Could we use StringUtils.isEmpty(getServiceUrl()) && StringUtils.isEmpty(getServiceUrlTls()) to reduce the codes ?

poorbarcode

Could you add a test for this change?

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

Used StringUtils.isEmpty(getServiceUrl()) && StringUtils.isEmpty(getServiceUrlTls()) to reduce the codes

harshithasudhakar · 2023-07-27T17:43:10Z

@poorbarcode I have made two testcases to make sure at least one URL is set.

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

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

Changes: 1. testUpdateClusterServiceUrl(): Tests API for updating cluster's Service URL, and checks if the retrieved serviceURL matches with it. 2. testUpdateClusterBrokerServiceUrl(): Tests API for updating cluster's Service Broker URL, and checks if the retrieved serviceBrokerURL matches with it.

harshithasudhakar · 2023-08-11T18:53:59Z

@poorbarcode @Technoboy-
I made the following changes in AdminApi2Test:

testUpdateClusterServiceUrl(): Tests API for updating cluster's Service URL, and checks if the retrieved serviceURL matches with it.
testUpdateClusterBrokerServiceUrl(): Tests API for updating cluster's Service Broker URL, and checks if the retrieved serviceBrokerURL matches with it.

harshithasudhakar · 2023-08-18T19:30:13Z

@poorbarcode @Technoboy- Hi, are there any other similar issues I can work on?

github-actions · 2023-09-18T01:47:37Z

The pr had no activity for 30 days, mark with Stale label.

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

poorbarcode · 2023-09-20T01:28:48Z

@harshithasudhakar

Please remove these useless imports.

https://github.com/apache/pulsar/actions/runs/6242793194/job/16947265796?pr=20821

Technoboy- · 2023-10-11T03:05:23Z

@poorbarcode @Technoboy- Hi, are there any other similar issues I can work on?

Sorry for the late response. I think you can try to fix the flaky tests, it can help you understand the related function more clearly.

harshithasudhakar · 2023-10-14T13:09:59Z

@Technoboy- Thank you for helping me out with this PR. Sure, I'll get started on identifying and addressing these tests and will certainly reach out for guidance when needed. If you have any specific resources or tips for getting started with this task, please do to share them.

harshithasudhakar · 2024-06-16T07:00:09Z

Hi, I'd like to fix the errors in this PR but I'm unable to find the log files for this. Can you help me out? How can I run the tests again?

Update ClusterDataImpl.java

404fbd2

Checking if `serviceUrl`, `serviceUrlTls`, `brokerServiceUrl`, or `brokerServiceUrlTls` are not null. Then proceeds to throw `IllegalArgumentException` if either of `serviceUrl`, `serviceUrlTls` or `brokerServiceUrl`, `brokerServiceUrlTls` has not been set.

github-actions bot added the doc-label-missing label Jul 17, 2023

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jul 17, 2023

harshithasudhakar changed the title ~~Update ClusterDataImpl.java~~ [Enhancement] Verify at least one URL is present for cluster creation Jul 17, 2023

Technoboy- reviewed Jul 24, 2023

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java Outdated Show resolved Hide resolved

Technoboy- reviewed Jul 24, 2023

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java Outdated Show resolved Hide resolved

Update ClusterDataImpl.java

b9c8f72

Technoboy- reviewed Jul 24, 2023

View reviewed changes

poorbarcode reviewed Jul 24, 2023

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java Outdated Show resolved Hide resolved

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java Outdated Show resolved Hide resolved

Update ClusterDataImpl.java

a105a47

Used StringUtils.isEmpty(getServiceUrl()) && StringUtils.isEmpty(getServiceUrlTls()) to reduce the codes

poorbarcode assigned harshithasudhakar Jul 24, 2023

poorbarcode added this to the 3.2.0 milestone Jul 24, 2023

poorbarcode added the area/config label Jul 24, 2023

Technoboy- changed the title ~~[Enhancement] Verify at least one URL is present for cluster creation~~ [improve][common] Verify at least one URL is present for cluster creation Jul 24, 2023

harshithasudhakar added 2 commits July 27, 2023 21:28

Update ClusterDataImpl.java

e6dfcf8

Update ClusterDataImplTest.java

154d259

harshithasudhakar requested review from Technoboy- and poorbarcode July 27, 2023 17:43

Update ClusterDataImplTest.java

41d5a4e

poorbarcode reviewed Jul 27, 2023

View reviewed changes

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

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java Outdated Show resolved Hide resolved

harshithasudhakar added 2 commits July 27, 2023 23:31

Update ClusterDataImpl.java

518b0d5

Update AdminApi2Test.java

b97a9d5

harshithasudhakar requested a review from poorbarcode August 12, 2023 20:38

poorbarcode approved these changes Sep 20, 2023

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java Outdated Show resolved Hide resolved

poorbarcode added the ready-to-test label Sep 20, 2023

poorbarcode closed this Sep 20, 2023

poorbarcode reopened this Sep 20, 2023

poorbarcode changed the title ~~[improve][common] Verify at least one URL is present for cluster creation~~ [improve] [broker] Verify at least one URL is present for cluster creation Sep 20, 2023

github-actions bot removed the Stale label Sep 20, 2023

harshithasudhakar and others added 4 commits September 21, 2023 14:44

Update ClusterDataImpl.java

b418367

Update ClusterDataImplTest.java

2a5e58d

Update ClusterDataImpl.java

db2ffdc

fix checkstyle issue

4bd190d

Technoboy- approved these changes Oct 11, 2023

View reviewed changes

Technoboy- added 2 commits October 11, 2023 11:17

fix test issue

190e832

fix test

5193fc4

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 11, 2024

lhotari added the release/4.0.1 label Oct 11, 2024

lhotari added release/4.0.2 and removed release/4.0.1 labels Dec 6, 2024

lhotari added release/4.0.3 and removed release/4.0.2 labels Jan 6, 2025

lhotari added release/4.0.4 and removed release/4.0.3 labels Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve] [broker] Verify at least one URL is present for cluster creation #20821

[improve] [broker] Verify at least one URL is present for cluster creation #20821

harshithasudhakar commented Jul 17, 2023 •

edited

Loading

github-actions bot commented Jul 17, 2023

Technoboy- left a comment

poorbarcode left a comment

harshithasudhakar commented Jul 27, 2023

harshithasudhakar commented Aug 11, 2023 •

edited

Loading

harshithasudhakar commented Aug 18, 2023

github-actions bot commented Sep 18, 2023

poorbarcode commented Sep 20, 2023

Technoboy- commented Oct 11, 2023

harshithasudhakar commented Oct 14, 2023

harshithasudhakar commented Jun 16, 2024

[improve] [broker] Verify at least one URL is present for cluster creation #20821

Are you sure you want to change the base?

[improve] [broker] Verify at least one URL is present for cluster creation #20821

Conversation

harshithasudhakar commented Jul 17, 2023 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

github-actions bot commented Jul 17, 2023

Technoboy- left a comment

Choose a reason for hiding this comment

poorbarcode left a comment

Choose a reason for hiding this comment

harshithasudhakar commented Jul 27, 2023

harshithasudhakar commented Aug 11, 2023 • edited Loading

harshithasudhakar commented Aug 18, 2023

github-actions bot commented Sep 18, 2023

poorbarcode commented Sep 20, 2023

Technoboy- commented Oct 11, 2023

harshithasudhakar commented Oct 14, 2023

harshithasudhakar commented Jun 16, 2024

harshithasudhakar commented Jul 17, 2023 •

edited

Loading

harshithasudhakar commented Aug 11, 2023 •

edited

Loading