Skip to content

[branch-2.11][fix][broker] Fix create ns - #18755

Merged
codelipenghui merged 2 commits into
apache:branch-2.11from
nodece:fix-create-ns-2.11
Dec 8, 2022
Merged

[branch-2.11][fix][broker] Fix create ns#18755
codelipenghui merged 2 commits into
apache:branch-2.11from
nodece:fix-create-ns-2.11

Conversation

@nodece

@nodece nodece commented Dec 6, 2022

Copy link
Copy Markdown
Member

Motivation

Cherry-pick #17864

The bundles policy of the namespace is ignored if the namespace is created directly by the NamespaceResources, see the logic of admin service:

private Policies getDefaultPolicesIfNull(Policies policies) {
if (policies == null) {
policies = new Policies();
}
int defaultNumberOfBundles = config().getDefaultNumberOfNamespaceBundles();
if (policies.bundles == null) {
policies.bundles = getBundles(defaultNumberOfBundles);
}
return policies;
}

Documentation

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

Matching PR in forked repository

PR in forked repository:

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 6, 2022
@Technoboy- Technoboy- changed the title [fix][broker][branch-2.11] Fix create ns [branch-2.11][fix][broker] Fix create ns Dec 6, 2022
@Technoboy- Technoboy- closed this Dec 6, 2022
@Technoboy- Technoboy- reopened this Dec 6, 2022
@nodece

nodece commented Dec 6, 2022

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

1 similar comment
@nodece

nodece commented Dec 7, 2022

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@nicoloboschi

Copy link
Copy Markdown
Contributor

@nodece this test is failing:

PulsarStandaloneTest.testCreateNameSpace
Cannot invoke "org.apache.pulsar.client.admin.PulsarAdmin.namespaces()" because the return value of "org.apache.pulsar.broker.PulsarService.getAdminClient()" is null

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@codelipenghui
codelipenghui merged commit 53f1cb2 into apache:branch-2.11 Dec 8, 2022
@BewareMyPower

Copy link
Copy Markdown
Contributor

@codelipenghui We should not merge a PR whose CI failed. This PR introduced a breaking change. See https://github.com/apache/pulsar/actions/runs/3637168979/jobs/6137886957.

BewareMyPower added a commit to BewareMyPower/pulsar that referenced this pull request Dec 9, 2022
BewareMyPower added a commit that referenced this pull request Dec 9, 2022
@BewareMyPower

Copy link
Copy Markdown
Contributor

Revert this PR to fix the broken branch-2.11. We should not cherry-pick #17864. Instead, we should use a correct way to fix the default bundle policy change (which should not be a big issue)

See https://lists.apache.org/thread/5orjrbrhomf6bt9stdfsv6z4bzs3m7w0

@nodece

nodece commented Dec 9, 2022

Copy link
Copy Markdown
Member Author

Revert this PR to fix the broken branch-2.11. We should not cherry-pick #17864. Instead, we should use a correct way to fix the default bundle policy change (which should not be a big issue)

See https://lists.apache.org/thread/5orjrbrhomf6bt9stdfsv6z4bzs3m7w0

I don't think this PR brings the breaking change. When you enable the authentication/authorization, you must configure the brokerClientAuthenticationPlugin and brokerClientAuthenticationParameters in the broke config file.

@BewareMyPower

Copy link
Copy Markdown
Contributor

I see your comments in the mail list, let's continue the discussion in the mail list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

6 participants