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.

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

Conversation

nodece
Copy link
Member

@nodece nodece commented Mar 9, 2023

Motivation

When creating the cluster, if the serviceUrlTls is empty in the ClusterData, I got Illegal service tls url, example: https://pulsar.example.com:8443. I expect the Pulsar works fine.

#19151 breaks this behavior, see the following code:

    public static void checkURIIfPresent(@Nullable String uri,
                                         @Nonnull Predicate<URI> predicate,
                                         @Nullable String errorMessage) throws IllegalArgumentException {
        if (uri == null) {
            return;
        }
        checkURI(uri, predicate, errorMessage);
    }

This check should also ignore the empty URL.

Modifications

  • Ignore checking the empty URL

Verifying this change

  • Make sure that the change passes the CI checks.

Added testCreateCluster test.

Documentation

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

@nodece nodece added this to the 3.0.0 milestone Mar 9, 2023
@nodece nodece self-assigned this Mar 9, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 9, 2023
@nodece nodece added area/broker ready-to-test and removed doc-not-needed Your PR changes do not impact docs labels Mar 9, 2023
@nodece
Copy link
Member Author

nodece commented Mar 9, 2023

/pulsarbot rerun-failure-checks

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 9, 2023
@nodece
Copy link
Member Author

nodece commented Mar 9, 2023

/pulsarbot rerun-failure-checks

@nodece nodece force-pushed the fix-cluster-with-empty-url branch from 5766204 to 472687a Compare March 10, 2023 08:12
@Technoboy-
Copy link
Contributor

/pulsarbot rerun-failure-checks

1 similar comment
@Technoboy-
Copy link
Contributor

/pulsarbot rerun-failure-checks

@nodece nodece force-pushed the fix-cluster-with-empty-url branch from 472687a to 6a0fd33 Compare March 15, 2023 08:00
@Technoboy-
Copy link
Contributor

/pulsarbot rerun-failure-checks

@nodece nodece force-pushed the fix-cluster-with-empty-url branch from d7d7782 to c1e495d Compare March 17, 2023 15:12
@nodece nodece force-pushed the fix-cluster-with-empty-url branch from c1e495d to 3ec995c Compare March 17, 2023 16:47
@Technoboy- Technoboy- merged commit 7f5ba2b into apache:master Mar 20, 2023
@@ -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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants