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

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

Merged
merged 1 commit into from
Mar 20, 2023
+37 −1
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiClusterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertThrows;
import static org.testng.Assert.fail;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.UUID;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -107,4 +109,36 @@ public void testDeleteExistFailureDomain() throws PulsarAdminException {

admin.clusters().deleteFailureDomain(CLUSTER, domainName);
}

@Test
public void testCreateCluster() throws PulsarAdminException {
List<ClusterData> clusterDataList = new ArrayList<>();
clusterDataList.add(ClusterData.builder()
.serviceUrl("http://pulsar.app:8080")
.serviceUrlTls("")
.brokerServiceUrl("pulsar://pulsar.app:6650")
.brokerServiceUrlTls("")
.build());
clusterDataList.add(ClusterData.builder()
.serviceUrl("")
.serviceUrlTls("https://pulsar.app:8443")
.brokerServiceUrl("")
.brokerServiceUrlTls("pulsar+ssl://pulsar.app:6651")
.build());
clusterDataList.add(ClusterData.builder()
.serviceUrl("")
.serviceUrlTls("")
.brokerServiceUrl("")
.brokerServiceUrlTls("")
.build());
clusterDataList.add(ClusterData.builder()
.serviceUrl(null)
.serviceUrlTls(null)
.brokerServiceUrl(null)
.brokerServiceUrlTls(null)
.build());
for (int i = 0; i < clusterDataList.size(); i++) {
admin.clusters().createCluster("cluster-test-" + i, clusterDataList.get(i));
}
}
}
2 changes: 1 addition & 1 deletion pulsar-common/src/main/java/org/apache/pulsar/common/util/URIPreconditions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static void checkURIIfPresent(@Nullable String uri,
public static void checkURIIfPresent(@Nullable String uri,
@Nonnull Predicate<URI> predicate,
@Nullable String errorMessage) throws IllegalArgumentException {
if (uri == null) {
if (uri == null || uri.length() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I think one risk here is that it is now possible to create a cluster with all empty cluster urls. I think it might make sense to verify that there is some URL that is not zero length.

Additionally, one benefit of this change is for clusters that only want to supply a TLS url.

What is your opinion @nodece?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think one risk here is that it is now possible to create a cluster with all empty cluster urls. I think it might make sense to verify that there is some URL that is not zero length.

Sure, I agreed with you! We must ensure that at least one URL is a non-zero length when creating the cluster.

Do you make a PR to do that? Recently, I don't have time to maintain the Pulsar :(

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaeljmarshall

Copy link
Member

Choose a reason for hiding this comment

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

I'll create a GitHub issue and label it as a good one for first time contributors.

Copy link
Member

Choose a reason for hiding this comment

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

I created #19866.

Also, while looking into it, I realized that this PR didn't introduce the possibility of configuring no urls. The issue I created is very low priority.

return;
}
checkURI(uri, predicate, errorMessage);
Expand Down
2 changes: 2 additions & 0 deletions pulsar-common/src/test/java/org/apache/pulsar/common/util/URIPreconditionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public void testCheckURI() {
// normal
checkURI("http://pulsar.apache.org", uri -> true);
checkURI("http://pulsar.apache.org", uri -> Objects.equals(uri.getScheme(), "http"));
checkURI("", uri -> true);
// illegal
try {
checkURI("pulsar.apache.org", uri -> Objects.equals(uri.getScheme(), "http"));
Expand All @@ -48,6 +49,7 @@ public void testCheckURI() {
@Test
public void testCheckURIIfPresent() {
checkURIIfPresent(null, uri -> false);
checkURIIfPresent("", uri -> false);
checkURIIfPresent("http://pulsar.apache.org", uri -> true);
try {
checkURIIfPresent("http/pulsar.apache.org", uri -> uri.getScheme() != null, "Error");
Expand Down