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 2020/11/07 03:35:07 UTC
[GitHub] [pulsar] jiazhai opened a new pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
jiazhai opened a new pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472
Fixes #8471
### Motivation
command bin/pulsar-admin ns-isolation-policy brokers not handled the "isPrimary" parameter. and caused always "false" output.
### Modifications
fix command,
add test.
### Verifying this change
new added test passed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520294759
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
Oh, I mean, do we need to add a field `isSecondary` to this entity? @jiazhai
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520294759
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
do we need to add a field `isSecondary` to this entity? @jiazhai
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520294759
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
do we need to add a field `isSecondary` to this entity? @jiazhai The `isPrimary` field also seems to be newly added https://github.com/apache/pulsar/pull/5314
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520293194
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#issuecomment-723561021
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#issuecomment-723739357
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520293927
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
Oh, @tuteng I see, there is no such an area for `isSecondary`. Maybe we could add this area if there is requirement from user.
```
public class BrokerNamespaceIsolationData {
@ApiModelProperty(
name = "brokerName",
value = "The broker name",
example = "broker1:8080"
)
public String brokerName;
@ApiModelProperty(
name = "policyName",
value = "Policy name",
example = "my-policy"
)
public String policyName;
@ApiModelProperty(
name = "isPrimary",
value = "Is Primary broker",
example = "true/false"
)
public boolean isPrimary;
@ApiModelProperty(
name = "namespaceRegex",
value = "The namespace-isolation policies attached to this broker"
)
public List<String> namespaceRegex;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520294759
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
do we need to add a field `isSecondary` to this entity? @jiazhai The `isPrimary` field also seems to be newly added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520293927
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
Oh, I see, there is no such an area for `isSecondary`
```
public class BrokerNamespaceIsolationData {
@ApiModelProperty(
name = "brokerName",
value = "The broker name",
example = "broker1:8080"
)
public String brokerName;
@ApiModelProperty(
name = "policyName",
value = "Policy name",
example = "my-policy"
)
public String policyName;
@ApiModelProperty(
name = "isPrimary",
value = "Is Primary broker",
example = "true/false"
)
public boolean isPrimary;
@ApiModelProperty(
name = "namespaceRegex",
value = "The namespace-isolation policies attached to this broker"
)
public List<String> namespaceRegex;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520292344
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
Do we need to add the initialization of `isSecondary`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472#discussion_r520294759
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java
##########
@@ -595,6 +595,9 @@ public NamespaceIsolationData getNamespaceIsolationPolicy(
brokerIsolationData.namespaceRegex = Lists.newArrayList();
}
brokerIsolationData.namespaceRegex.addAll(policyData.namespaces);
+ if (nsPolicyImpl.isPrimaryBroker(broker)) {
+ brokerIsolationData.isPrimary = true;
+ }
Review comment:
do we need to add a field `isSecondary` to this entity? @jiazhai The `isPrimary` field also seems to be newly added https://github.com/apache/pulsar/pull/5314, Or add it in the future
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #8472: [Issue 8471] command getBrokersWithNamespaceIsolationPolicy, add test
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #8472:
URL: https://github.com/apache/pulsar/pull/8472
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org