You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/28 02:11:29 UTC
[GitHub] [pulsar] nodece opened a new pull request, #17864: [fix][broker] Fix create ns
nodece opened a new pull request, #17864:
URL: https://github.com/apache/pulsar/pull/17864
### Motivation
Namespace policy is ignored if the namespace is created directly by the `NamespaceResources`.
### Modifications
- Use `admin` to create a namespace instead of the `NamespaceResources`
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
### Matching PR in forked repository
https://github.com/nodece/pulsar/pull/4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1344000787
> If only the number of bundles were affected, could you use another solution?
No.
> This PR brings a breaking change to the standalone. We need to add an integration test to protect it.
Good catch. Let me add an integration test for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#discussion_r1044265557
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java:
##########
@@ -394,9 +392,7 @@ void createNameSpace(String cluster, String publicTenant, NamespaceName ns) thro
}
if (!nsr.namespaceExists(ns)) {
- Policies nsp = new Policies();
- nsp.replication_clusters = Collections.singleton(config.getClusterName());
- nsr.createPolicies(ns, nsp);
Review Comment:
So far, I've only found the bundle issue, I'm not sure about the other factors, so using the `admin`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1344009859
For how to reproduce it, you can reference the [pulsar-test-service-start.sh](https://github.com/apache/pulsar/blob/branch-2.11/pulsar-client-cpp/pulsar-test-service-start.sh), which uses [standalone-ssl.conf](https://github.com/apache/pulsar/blob/branch-2.11/pulsar-client-cpp/test-conf/standalone-ssl.conf) as the config file. In additional, even if `Pulsar service is ready` was printed, the standalone actually didn't start successfully. You can log in the docker container and run `bin/pulsar standalone -nss -nfw` directly, you should see the following logs:
```
2022-12-09T07:45:12,311+0000 [main] INFO org.apache.pulsar.broker.PulsarService - created admin with url http://localhost:8080
2022-12-09T07:45:12,664+0000 [pulsar-web-46-15] ERROR org.apache.pulsar.broker.admin.v2.Namespaces - [anonymous] Failed to create namespace public/default
java.util.concurrent.CompletionException: org.apache.pulsar.broker.web.RestException: Unauthorized to validateTenantOperation for originalPrincipal [null] and clientAppId [anonymous] about operation [CREATE_NAMESPACE] on tenant [public]
at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[?:?]
at java.util.concurrent.CompletableFuture.uniAcceptNow(CompletableFuture.java:761) ~[?:?]
at java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:735) ~[?:?]
at java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2182) ~[?:?]
at org.apache.pulsar.broker.web.PulsarWebResource.validateTenantOperationAsync(PulsarWebResource.java:995) ~[org.apache.pulsar-pulsar-broker-2.11.0.jar:2.11.0]
at org.apache.pulsar.broker.admin.impl.NamespacesBase.internalCreateNamespace(NamespacesBase.java:138) ~[org.apache.pulsar-pulsar-broker-2.11.0.jar:2.11.0]
at org.apache.pulsar.broker.admin.v2.Namespaces.createNamespace(Namespaces.java:166) ~[org.apache.pulsar-pulsar-broker-2.11.0.jar:2.11.0]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1344072690
It's a real-world breaking change, not something like "it's an API that might already be used somewhere". The current semantics have already been applied in the standalone deployment of the C++/Python client tests.
I just confirmed that the Golang client tests would also fail after this change.
https://github.com/apache/pulsar-client-go/blob/055b00b83ccfef4ea12a593e3678cb4bdd18311c/integration-tests/conf/standalone.conf#L112-L115
Other ecosystems are not confirmed. But I think it's enough.
Breaking change is a breaking change not for "how it should be used", but for "whether it has already been used".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece merged pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece merged PR #17864:
URL: https://github.com/apache/pulsar/pull/17864
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#discussion_r984389298
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java:
##########
@@ -394,9 +392,7 @@ void createNameSpace(String cluster, String publicTenant, NamespaceName ns) thro
}
if (!nsr.namespaceExists(ns)) {
- Policies nsp = new Policies();
- nsp.replication_clusters = Collections.singleton(config.getClusterName());
- nsr.createPolicies(ns, nsp);
+ broker.getAdminClient().namespaces().createNamespace(ns.toString());
Review Comment:
Some policy is ignored because the conf file effects this policy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1263767295
> Lgtm
>
> What about adding a test case?
I think we should add an integration test, what do you think about that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1344014750
Because you didn't setup `brokerClientAuthenticationPlugin` and `brokerClientAuthenticationParameters` in the `https://github.com/apache/pulsar/blob/branch-2.11/pulsar-client-cpp/test-conf/standalone-ssl.conf`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1343996708
> Could you clarify which policies are affected?
See the PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#discussion_r1044251221
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java:
##########
@@ -394,9 +392,7 @@ void createNameSpace(String cluster, String publicTenant, NamespaceName ns) thro
}
if (!nsr.namespaceExists(ns)) {
- Policies nsp = new Policies();
- nsp.replication_clusters = Collections.singleton(config.getClusterName());
- nsr.createPolicies(ns, nsp);
Review Comment:
Why not fix it by something like:
```java
nsp.bundles = /* ... */;
```
It would be more clear for what's the default bundle policy of the standalone and not affected by the PulsarAdmin implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1343973197
Could you clarify which policies are affected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1343998252
If only the number of bundles were affected, could you use another solution? This PR brings a breaking change to the standalone. We need to add an integration test to protect it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#issuecomment-1344064977
> Because you didn't set up the brokerClientAuthenticationPlugin and brokerClientAuthenticationParameters in the [standalone-ssl.conf](https://github.com/apache/pulsar/blob/branch-2.11/pulsar-client-cpp/test-conf/standalone-ssl.conf).
The previous deployment also didn't set up these two configs and it has worked for a long time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #17864: [fix][broker] Fix create ns
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17864:
URL: https://github.com/apache/pulsar/pull/17864#discussion_r984384060
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java:
##########
@@ -394,9 +392,7 @@ void createNameSpace(String cluster, String publicTenant, NamespaceName ns) thro
}
if (!nsr.namespaceExists(ns)) {
- Policies nsp = new Policies();
- nsp.replication_clusters = Collections.singleton(config.getClusterName());
- nsr.createPolicies(ns, nsp);
+ broker.getAdminClient().namespaces().createNamespace(ns.toString());
Review Comment:
Sorry, I don't get it, why `nsr.createPolicies(ns, nsp);` is ignored?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org