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/12/27 11:13:00 UTC
[GitHub] [pulsar] gaozhangmin opened a new pull request #13525: Healthcheck v2 failed
gaozhangmin opened a new pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525
fix #13520
--
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 a change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r776694978
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2955,7 +2956,7 @@ public CompactedTopic getCompactedTopic() {
@Override
public boolean isSystemTopic() {
- return false;
+ return HEARTBEAT_TENANT.equals(TopicName.get(topic).getTenant());
Review comment:
#13495 made changes on `getReplicationClusters` . don't need this change any more
--
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] massakam merged pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
massakam merged pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525
--
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] eolivelli commented on a change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r779476423
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -1371,6 +1373,12 @@ public void closeFailed(ManagedLedgerException exception, Object ctx) {
if (!name.isGlobal()) {
return CompletableFuture.completedFuture(null);
}
+ NamespaceName heartbeatNamespace = NamespaceService.getHeartbeatNamespaceV2(
Review comment:
we move this into the PulsarService as a field, this way we don't have to create this every 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] gaozhangmin edited a comment on pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin edited a comment on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1001902512
@liudezhi2098 The test is already covered by `org.apache.pulsar.broker.admin.AdminApiHealthCheckTest#testHealthCheckupV2`
The previous unit test `testHealthCheckupV2 ` created a heartbeat namespace Manually before running health check.
But, In fact, the heartbeat namespace would not be created in cluster.
--
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] massakam commented on pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
massakam commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1005447639
I think `NonPersistentTopic.java` should be modified as well as `PersistentTopic.java`.
https://github.com/apache/pulsar/blob/ef42af2803e0d8b8141331807a0549c4d3277361/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java#L521-L525
--
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 a change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r775752192
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -1427,7 +1428,7 @@ public void closeFailed(ManagedLedgerException exception, Object ctx) {
@VisibleForTesting
public CompletableFuture<List<String>> getReplicationClusters(TopicName topicName) {
CompletableFuture<Optional<TopicPolicies>> future = new CompletableFuture<>();
- if (isSystemTopic()) {
+ if (isSystemTopic() || HEARTBEAT_TENANT.equals(topicName.getTenant())) {
Review comment:
done @Jason918
--
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 #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1003000903
> This PR looks like adding some tweaks specific for this special topic.
@eolivelli The direct reason is that heartbeat namespace was not created. just registered by `registerBootstrapNamespaces` during startup. The optional fix maybe, we can create this heartbeat namespace during startup.
--
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 #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1003006457
/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] massakam commented on a change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
massakam commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r779288016
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -66,18 +63,7 @@ public void testHealthCheckupV1() throws Exception {
admin.brokers().healthcheck(TopicVersion.V1);
}
- @Test(expectedExceptions = PulsarAdminException.class)
- public void testHealthCheckupV2Error() throws Exception {
- admin.brokers().healthcheck(TopicVersion.V2);
- }
-
- @Test
Review comment:
`@Test` annotation is missing.
--
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 #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1007085348
/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] gaozhangmin edited a comment on pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin edited a comment on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1001902512
@liudezhi2098 The test is already covered by `org.apache.pulsar.broker.admin.AdminApiHealthCheckTest#testHealthCheckupV2`
The previous unit test `testHealthCheckupV2 ` created a heartbeat namespace Manually before running health check.
But, In fact, the heartbeat namespace would not be created in cluster，so, no policies znode under admin. that's why this issue happened
--
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 #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1001902512
@liudezhi2098 The test is already covered by `org.apache.pulsar.broker.admin.AdminApiHealthCheckTest#testHealthCheckupV2Error`
The previous unit test `testHealthCheckupV2 ` created a heartbeat namespace Manually before running health check.
But, In fact, the heartbeat namespace would not be created in cluster.
--
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 #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1005427370
> There may also be namespaces under the `pulsar` tenant that are not for healthcheck. For example, it is possible for a user to create a namespace named `pulsar/test`. For such namespaces, we should not skip these checks.
Fixed in the last commit.
--
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 a change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r776694978
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2955,7 +2956,7 @@ public CompactedTopic getCompactedTopic() {
@Override
public boolean isSystemTopic() {
- return false;
+ return HEARTBEAT_TENANT.equals(TopicName.get(topic).getTenant());
Review comment:
#13495 made changes on `getReplicationClusters` don't need this change any more
--
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] massakam merged pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
massakam merged pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525
--
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] gaoran10 commented on pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1066597807
The https://github.com/apache/pulsar/pull/14658 cherry-pick this PR to `branch-2.9`.
--
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] massakam commented on pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
massakam commented on pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#issuecomment-1005375005
There may also be namespaces under the `pulsar` tenant that are not for healthcheck. For example, it is possible for a user to create a namespace named `pulsar/test`. For such namespaces, we should not skip these 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] Jason918 commented on a change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r776125144
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2955,7 +2956,7 @@ public CompactedTopic getCompactedTopic() {
@Override
public boolean isSystemTopic() {
- return false;
+ return HEARTBEAT_TENANT.equals(TopicName.get(topic).getTenant());
Review comment:
Not like this. There are some other system topics.
The origin check seems to be `org.apache.pulsar.broker.service.BrokerService#isSystemTopic`.
--
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 change in pull request #13525: Healthcheck v2 failed
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #13525:
URL: https://github.com/apache/pulsar/pull/13525#discussion_r775705434
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -1427,7 +1428,7 @@ public void closeFailed(ManagedLedgerException exception, Object ctx) {
@VisibleForTesting
public CompletableFuture<List<String>> getReplicationClusters(TopicName topicName) {
CompletableFuture<Optional<TopicPolicies>> future = new CompletableFuture<>();
- if (isSystemTopic()) {
+ if (isSystemTopic() || HEARTBEAT_TENANT.equals(topicName.getTenant())) {
Review comment:
Can we include heartbeat topic in SystemTopic?
--
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