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 2021/08/12 01:42:33 UTC
[GitHub] [pulsar] kaushik-develop opened a new pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
kaushik-develop opened a new pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643
…egisters
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
### Motivation
To avoid aliasing, use the fully-qualified namespace name in register/unregister of namespaces to RGs.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as *(please describe tests)*.
mvn test -Dtest=org.apache.pulsar.broker.resourcegroup.ResourceGroupServiceTest -pl pulsar-broker
mvn test -Dtest=org.apache.pulsar.broker.resourcegroup.ResourceGroupUsageAggregationTest -pl pulsar-broker
mvn test -Dtest=org.apache.pulsar.broker.resourcegroup.RGUsageMTAggrWaitForAllMesgsTest -pl pulsar-broker
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
#### For contributor
For this PR, do we need to update docs?
No
This is an internal change in how resource-groups work.
#### For committer
For this PR, do we need to update docs?
- If yes,
- if you update docs in this PR, label this PR with the `doc` label.
- if you plan to update docs later, label this PR with the `doc-required` label.
- if you need help on updating docs, create a follow-up issue with the `doc-required` label.
- If no, label this PR with the `no-need-doc` label and explain why.
--
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] kaushik-develop commented on pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643#issuecomment-897866903
/pulsarbot run-failure-checks
--
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] jerrypeng merged pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
jerrypeng merged pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643
--
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] kaushik-develop commented on a change in pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on a change in pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643#discussion_r687935424
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupService.java
##########
@@ -233,66 +235,80 @@ public void unRegisterTenant(String resourceGroupName, String tenantName) throws
* Registers a namespace as a user of a resource group.
*
* @param resourceGroupName
- * @param namespaceName
+ * @param fqNamespaceName (i.e., in "tenant/Namespace" format)
* @throws if the RG does not exist, or if the NS already references the RG.
*/
- public void registerNameSpace(String resourceGroupName, String namespaceName) throws PulsarAdminException {
+ public void registerNameSpace(String resourceGroupName, String fqNamespaceName) throws PulsarAdminException {
+ // Since it is a fully qualified NS, we expect a '/' in it.
+ String[] parts = fqNamespaceName.split("/");
Review comment:
Thanks; updated in the next iteration.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupService.java
##########
@@ -233,66 +235,80 @@ public void unRegisterTenant(String resourceGroupName, String tenantName) throws
* Registers a namespace as a user of a resource group.
*
* @param resourceGroupName
- * @param namespaceName
+ * @param fqNamespaceName (i.e., in "tenant/Namespace" format)
* @throws if the RG does not exist, or if the NS already references the RG.
*/
- public void registerNameSpace(String resourceGroupName, String namespaceName) throws PulsarAdminException {
+ public void registerNameSpace(String resourceGroupName, String fqNamespaceName) throws PulsarAdminException {
+ // Since it is a fully qualified NS, we expect a '/' in it.
+ String[] parts = fqNamespaceName.split("/");
+ if (parts.length < 2) {
+ String errMesg = "Fully-qualified namespace " + fqNamespaceName + " does not have a topic in it";
+ throw new PulsarAdminException(errMesg);
+ }
+
ResourceGroup rg = checkResourceGroupExists(resourceGroupName);
// Check that the NS-name doesn't already have a RG association.
// [If it does, that should be unregistered before putting a different association.]
- ResourceGroup oldRG = this.namespaceToRGsMap.get(namespaceName);
+ ResourceGroup oldRG = this.namespaceToRGsMap.get(fqNamespaceName);
if (oldRG != null) {
- String errMesg = "Namespace " + namespaceName + " already references a resource group: " + oldRG.getID();
+ String errMesg = "Namespace " + fqNamespaceName + " already references a resource group: " + oldRG.getID();
throw new PulsarAdminException(errMesg);
}
- ResourceGroupOpStatus status = rg.registerUsage(namespaceName, ResourceGroupRefTypes.Namespaces, true,
+ ResourceGroupOpStatus status = rg.registerUsage(fqNamespaceName, ResourceGroupRefTypes.Namespaces, true,
this.resourceUsageTransportManagerMgr);
if (status == ResourceGroupOpStatus.Exists) {
String errMesg = String.format("Namespace {} already references the target resource group {}",
- namespaceName, resourceGroupName);
+ fqNamespaceName, resourceGroupName);
throw new PulsarAdminException(errMesg);
}
// Associate this NS-name with the RG.
- this.namespaceToRGsMap.put(namespaceName, rg);
+ this.namespaceToRGsMap.put(fqNamespaceName, rg);
rgNamespaceRegisters.labels(resourceGroupName).inc();
}
- /**
- * Return the resource group associated with a namespace.
- *
- * @param namespaceName
- * @throws if the RG does not exist, or if the NS already references the RG.
- */
- public ResourceGroup getNamespaceResourceGroup(String namespaceName) {
- return this.namespaceToRGsMap.get(namespaceName);
- }
-
/**
* UnRegisters a namespace from a resource group.
*
* @param resourceGroupName
- * @param namespaceName
+ * @param fqNamespaceName i.e., in "tenant/Namespace" format)
* @throws if the RG does not exist, or if the NS does not references the RG yet.
*/
- public void unRegisterNameSpace(String resourceGroupName, String namespaceName) throws PulsarAdminException {
+ public void unRegisterNameSpace(String resourceGroupName, String fqNamespaceName) throws PulsarAdminException {
+ // Since it is a fully qualified NS, we expect a '/' in it.
+ String[] parts = fqNamespaceName.split("/");
Review comment:
Thanks; updated in the next iteration.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -60,7 +68,11 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
- return (long) calculatedQuota;
+ val longCalculatedQuota = (long) calculatedQuota;
Review comment:
No explicit need. I am just more used to the deciphering of types (through my C++11 and later background) by compiler support, instead of by the programmer.
--
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] jerrypeng commented on a change in pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643#discussion_r687390076
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupService.java
##########
@@ -233,66 +235,80 @@ public void unRegisterTenant(String resourceGroupName, String tenantName) throws
* Registers a namespace as a user of a resource group.
*
* @param resourceGroupName
- * @param namespaceName
+ * @param fqNamespaceName (i.e., in "tenant/Namespace" format)
* @throws if the RG does not exist, or if the NS already references the RG.
*/
- public void registerNameSpace(String resourceGroupName, String namespaceName) throws PulsarAdminException {
+ public void registerNameSpace(String resourceGroupName, String fqNamespaceName) throws PulsarAdminException {
+ // Since it is a fully qualified NS, we expect a '/' in it.
+ String[] parts = fqNamespaceName.split("/");
Review comment:
There is already code written to do this. Please use NamespaceName class to parse 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] jerrypeng commented on a change in pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643#discussion_r687391661
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -60,7 +68,11 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
- return (long) calculatedQuota;
+ val longCalculatedQuota = (long) calculatedQuota;
Review comment:
is there a reason we need to the lombok.val here instead of an explicit type?
--
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] kaushik-develop commented on a change in pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on a change in pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643#discussion_r687935502
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -60,7 +68,11 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
- return (long) calculatedQuota;
+ val longCalculatedQuota = (long) calculatedQuota;
Review comment:
No explicit need. I am just more used to the deciphering of types (through my C++11 and later background) by compiler support, instead of by the programmer, and the Pulsar style checks don't seem to prevent this usage.
--
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] jerrypeng commented on a change in pull request #11643: [PIP-82] [pulsar-broker] Use the qualified NS-name in RG-register/unr…
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #11643:
URL: https://github.com/apache/pulsar/pull/11643#discussion_r687390833
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupService.java
##########
@@ -233,66 +235,80 @@ public void unRegisterTenant(String resourceGroupName, String tenantName) throws
* Registers a namespace as a user of a resource group.
*
* @param resourceGroupName
- * @param namespaceName
+ * @param fqNamespaceName (i.e., in "tenant/Namespace" format)
* @throws if the RG does not exist, or if the NS already references the RG.
*/
- public void registerNameSpace(String resourceGroupName, String namespaceName) throws PulsarAdminException {
+ public void registerNameSpace(String resourceGroupName, String fqNamespaceName) throws PulsarAdminException {
+ // Since it is a fully qualified NS, we expect a '/' in it.
+ String[] parts = fqNamespaceName.split("/");
+ if (parts.length < 2) {
+ String errMesg = "Fully-qualified namespace " + fqNamespaceName + " does not have a topic in it";
+ throw new PulsarAdminException(errMesg);
+ }
+
ResourceGroup rg = checkResourceGroupExists(resourceGroupName);
// Check that the NS-name doesn't already have a RG association.
// [If it does, that should be unregistered before putting a different association.]
- ResourceGroup oldRG = this.namespaceToRGsMap.get(namespaceName);
+ ResourceGroup oldRG = this.namespaceToRGsMap.get(fqNamespaceName);
if (oldRG != null) {
- String errMesg = "Namespace " + namespaceName + " already references a resource group: " + oldRG.getID();
+ String errMesg = "Namespace " + fqNamespaceName + " already references a resource group: " + oldRG.getID();
throw new PulsarAdminException(errMesg);
}
- ResourceGroupOpStatus status = rg.registerUsage(namespaceName, ResourceGroupRefTypes.Namespaces, true,
+ ResourceGroupOpStatus status = rg.registerUsage(fqNamespaceName, ResourceGroupRefTypes.Namespaces, true,
this.resourceUsageTransportManagerMgr);
if (status == ResourceGroupOpStatus.Exists) {
String errMesg = String.format("Namespace {} already references the target resource group {}",
- namespaceName, resourceGroupName);
+ fqNamespaceName, resourceGroupName);
throw new PulsarAdminException(errMesg);
}
// Associate this NS-name with the RG.
- this.namespaceToRGsMap.put(namespaceName, rg);
+ this.namespaceToRGsMap.put(fqNamespaceName, rg);
rgNamespaceRegisters.labels(resourceGroupName).inc();
}
- /**
- * Return the resource group associated with a namespace.
- *
- * @param namespaceName
- * @throws if the RG does not exist, or if the NS already references the RG.
- */
- public ResourceGroup getNamespaceResourceGroup(String namespaceName) {
- return this.namespaceToRGsMap.get(namespaceName);
- }
-
/**
* UnRegisters a namespace from a resource group.
*
* @param resourceGroupName
- * @param namespaceName
+ * @param fqNamespaceName i.e., in "tenant/Namespace" format)
* @throws if the RG does not exist, or if the NS does not references the RG yet.
*/
- public void unRegisterNameSpace(String resourceGroupName, String namespaceName) throws PulsarAdminException {
+ public void unRegisterNameSpace(String resourceGroupName, String fqNamespaceName) throws PulsarAdminException {
+ // Since it is a fully qualified NS, we expect a '/' in it.
+ String[] parts = fqNamespaceName.split("/");
Review comment:
same here
--
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