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/04/21 09:27:01 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request, #15252: [improve][broker] Tidy up the system topic.
Technoboy- opened a new pull request, #15252:
URL: https://github.com/apache/pulsar/pull/15252
### Motivation
For #13520, #14643, #14949, we fix some issues related to system topic but result in checking the system topic in different method. So it's better to tidy up the system topic.
### Documentation
- [x] `no-need-doc`
(Please 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] congbobo184 commented on a diff in pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857056518
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1372,11 +1373,18 @@ public static String getSLAMonitorBrokerName(ServiceUnitId ns) {
}
public static boolean isSystemServiceNamespace(String namespace) {
- return HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
+ return SYSTEM_NAMESPACE.toString().equals(namespace)
+ || HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
Review Comment:
may can use isHeartbeatNamespace()
--
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 #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857100495
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTestBase.java:
##########
@@ -123,7 +125,7 @@ protected void setUpBase(int numBroker,int numPartitionsOfTC, String topic, int
admin.tenants().createTenant(NamespaceName.SYSTEM_NAMESPACE.getTenant(),
new TenantInfoImpl(Sets.newHashSet("appid1"), Sets.newHashSet(CLUSTER_NAME)));
admin.namespaces().createNamespace(NamespaceName.SYSTEM_NAMESPACE.toString());
- admin.topics().createPartitionedTopic(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString(), numPartitionsOfTC);
+ createTransactionCoordinatorAssign(numPartitionsOfTC);
Review Comment:
I have a question why don't use `admin.topics().createPartitionedTopic(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString(), numPartitionsOfTC);` to create topic.
--
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] Technoboy- commented on a diff in pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857087963
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1372,11 +1373,18 @@ public static String getSLAMonitorBrokerName(ServiceUnitId ns) {
}
public static boolean isSystemServiceNamespace(String namespace) {
- return HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
+ return SYSTEM_NAMESPACE.toString().equals(namespace)
+ || HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
Review Comment:
yes, right. Fixed.
--
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] Technoboy- commented on a diff in pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857101616
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerTestBase.java:
##########
@@ -50,6 +53,18 @@ private void baseSetupCommon() throws Exception {
admin.namespaces().setNamespaceReplicationClusters("prop/ns-abc", Sets.newHashSet("test"));
}
+ protected void createTransactionCoordinatorAssign() throws MetadataStoreException {
+ createTransactionCoordinatorAssign(1);
+ }
+
+ protected void createTransactionCoordinatorAssign(int partitions) throws MetadataStoreException {
+ pulsar.getPulsarResources()
+ .getNamespaceResources()
+ .getPartitionedTopicResources()
+ .createPartitionedTopic(SystemTopicNames.TRANSACTION_COORDINATOR_ASSIGN,
+ new PartitionedTopicMetadata(1));
Review Comment:
yes, right. Fix.
--
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 #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857131552
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTestBase.java:
##########
@@ -123,7 +125,7 @@ protected void setUpBase(int numBroker,int numPartitionsOfTC, String topic, int
admin.tenants().createTenant(NamespaceName.SYSTEM_NAMESPACE.getTenant(),
new TenantInfoImpl(Sets.newHashSet("appid1"), Sets.newHashSet(CLUSTER_NAME)));
admin.namespaces().createNamespace(NamespaceName.SYSTEM_NAMESPACE.toString());
- admin.topics().createPartitionedTopic(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString(), numPartitionsOfTC);
+ createTransactionCoordinatorAssign(numPartitionsOfTC);
Review Comment:
Thanks for your reply.
--
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] Technoboy- commented on a diff in pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857102157
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTestBase.java:
##########
@@ -123,7 +125,7 @@ protected void setUpBase(int numBroker,int numPartitionsOfTC, String topic, int
admin.tenants().createTenant(NamespaceName.SYSTEM_NAMESPACE.getTenant(),
new TenantInfoImpl(Sets.newHashSet("appid1"), Sets.newHashSet(CLUSTER_NAME)));
admin.namespaces().createNamespace(NamespaceName.SYSTEM_NAMESPACE.toString());
- admin.topics().createPartitionedTopic(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString(), numPartitionsOfTC);
+ createTransactionCoordinatorAssign(numPartitionsOfTC);
Review Comment:
Ah, because TRANSACTION_COORDINATOR_ASSIGN is not a topic in fact. If we use the old way to create it like `createPartitionedTopic `, we have to check the name on the broker side to let it pass(like not to create managed ledger or something).
--
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] Technoboy- commented on a diff in pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857123395
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1372,11 +1373,18 @@ public static String getSLAMonitorBrokerName(ServiceUnitId ns) {
}
public static boolean isSystemServiceNamespace(String namespace) {
- return HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
+ return SYSTEM_NAMESPACE.toString().equals(namespace)
+ || HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
Review Comment:
The test : OverloadShedderTest#testBrokerWithMultipleBundles could not pass. Because the bundle data is not regular(namespace/bundle), so only have to keep the original impmenetation.
--
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] Technoboy- commented on pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#issuecomment-1105163794
> @Technoboy- Why not add a method to check if a system topic is a transaction related topic?
We have `PulsarService#isTransactionSystemTopic`, I'm going to move this to a new class.
--
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 #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857100874
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerTestBase.java:
##########
@@ -50,6 +53,18 @@ private void baseSetupCommon() throws Exception {
admin.namespaces().setNamespaceReplicationClusters("prop/ns-abc", Sets.newHashSet("test"));
}
+ protected void createTransactionCoordinatorAssign() throws MetadataStoreException {
+ createTransactionCoordinatorAssign(1);
+ }
+
+ protected void createTransactionCoordinatorAssign(int partitions) throws MetadataStoreException {
+ pulsar.getPulsarResources()
+ .getNamespaceResources()
+ .getPartitionedTopicResources()
+ .createPartitionedTopic(SystemTopicNames.TRANSACTION_COORDINATOR_ASSIGN,
+ new PartitionedTopicMetadata(1));
Review Comment:
```suggestion
new PartitionedTopicMetadata(n));
```
--
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] Technoboy- commented on a diff in pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#discussion_r857123395
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1372,11 +1373,18 @@ public static String getSLAMonitorBrokerName(ServiceUnitId ns) {
}
public static boolean isSystemServiceNamespace(String namespace) {
- return HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
+ return SYSTEM_NAMESPACE.toString().equals(namespace)
+ || HEARTBEAT_NAMESPACE_PATTERN.matcher(namespace).matches()
Review Comment:
The test : OverloadShedderTest#testBrokerWithMultipleBundles could not pass. I change back to logic 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
[GitHub] [pulsar] codelipenghui merged pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15252:
URL: https://github.com/apache/pulsar/pull/15252
--
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] gaozhangmin commented on pull request #15252: [improve][broker] Tidy up the system topic.
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #15252:
URL: https://github.com/apache/pulsar/pull/15252#issuecomment-1104971955
@Technoboy- Why not add a method to check if a system topic is a transaction related topic?
--
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