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/06/18 21:23:06 UTC
[GitHub] [pulsar] aahmed-se opened a new pull request #10962: [WIP] Add v2 health admin endpoint
aahmed-se opened a new pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962
--
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] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655738605
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/TopicVersion.java
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.admin;
Review comment:
Moved it , still kept the name TopicVersion it's more clear.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
we create the v1 namespace by default
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdConsume.java
##########
@@ -414,7 +414,7 @@ private int consumeFromWebSocket(String topic) {
LOG.debug("No message to consume after waiting for 5 seconds.");
} else {
try {
- System.out.println(Base64.getDecoder().decode(msg));
+ System.out.println(Arrays.toString(Base64.getDecoder().decode(msg)));
Review comment:
to print out the string not the byte address.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
that's the present behaviour. it's a legacy decision.
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
##########
@@ -282,4 +282,5 @@ public static String getReasonFromServer(WebApplicationException e) {
}
}
}
+
Review comment:
removed
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
We already have a new version of the method that does that.
``` public static String getHeartbeatNamespaceV2(String host, ServiceConfiguration config) {
Integer port = null;
if (config.getWebServicePort().isPresent()) {
port = config.getWebServicePort().get();
} else if (config.getWebServicePortTls().isPresent()) {
port = config.getWebServicePortTls().get();
}
return String.format(HEARTBEAT_NAMESPACE_FMT_V2, host, port);
}
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
We already have a new version of the method that does that.
```
public static String getHeartbeatNamespaceV2(String host, ServiceConfiguration config) {
Integer port = null;
if (config.getWebServicePort().isPresent()) {
port = config.getWebServicePort().get();
} else if (config.getWebServicePortTls().isPresent()) {
port = config.getWebServicePortTls().get();
}
return String.format(HEARTBEAT_NAMESPACE_FMT_V2, host, port);
}
```
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
No the default endpoint is old format, only if they choose to use the new format by using the endpoint override do they need the new ns created. It shouldn't be created by default.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
No the default endpoint is using the old format, only if they choose to use the new format by using the endpoint override do they need to use the new namespace. It shouldn't be created by default.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
added 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655765724
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
Why does the v2 version fail but the v1 version passes without having to explicitly create the namespace the health check topic will be in?
--
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] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655777199
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
where are we creating 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655861542
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
@aahmed-se I am talking about `checkHeartbeatNamespace` not `getHeartbeatNamespace`
--
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] aahmed-se closed pull request #10962: [Broker] Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se closed pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962
--
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 #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655767044
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
##########
@@ -282,4 +282,5 @@ public static String getReasonFromServer(WebApplicationException e) {
}
}
}
+
Review comment:
nit unnecessary new line
--
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] cckellogg commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655724390
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/TopicVersion.java
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.admin;
Review comment:
Should we move this to this package `org.apache.pulsar.common.naming` and call it Version for NamingVersion?
--
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] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655800495
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
You also need to update the static variable `HEARTBEAT_NAMESPACE_PATTERN` and the method `checkHeartbeatNamespace` to support v2 version of the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aahmed-se commented on a change in pull request #10962: [WIP] Add v2 health admin endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r653924762
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,19 +292,19 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
+ private void healthCheck(AsyncResponse asyncResponse,
+ TopicVersion topicVersion) throws PulsarServerException {
+
validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
+ String topic;
+
+ if (topicVersion == TopicVersion.V1) {
+ String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ } else {
+ topic = ""; // TODO
Review comment:
Still have to implement it, looking to see what's the simplest way to do it.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,31 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/healthv2")
Review comment:
done
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,19 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/health")
Review comment:
that was there originally don't want to change the default behavior.
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -268,12 +268,12 @@
*
* @throws PulsarAdminException if the healthcheck fails.
*/
- void healthcheck() throws PulsarAdminException;
+ void healthcheck(String topicVersion) throws PulsarAdminException;
Review comment:
sure will keep them as deprecated
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -268,12 +268,12 @@
*
* @throws PulsarAdminException if the healthcheck fails.
*/
- void healthcheck() throws PulsarAdminException;
+ void healthcheck(String topicVersion) throws PulsarAdminException;
Review comment:
done
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +368,31 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
+ @Deprecated
public CompletableFuture<Void> healthcheckAsync() {
+ return healthcheckAsync("V1");
+ }
+
+ @Override
+ public void healthcheck(String topicVersion) throws PulsarAdminException {
Review comment:
sure where would be put the TopicVersion class that client, broker and tools package can share it.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,30 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic = null;
+ PulsarClient client = null;
+ try {
+ validateSuperUserAccess();
+
+ if (topicVersion == TopicVersion.V1) {
+ String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ } else {
+ LOG.info("Using healthCheck with V2 topic name");
Review comment:
done
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -352,9 +353,10 @@ public void failed(Throwable throwable) {
}
@Override
+ @Deprecated
public void healthcheck() throws PulsarAdminException {
try {
- healthcheckAsync().get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
+ healthcheckAsync("V1").get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
Review comment:
done
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +368,31 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
+ @Deprecated
public CompletableFuture<Void> healthcheckAsync() {
+ return healthcheckAsync("V1");
Review comment:
done
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +387,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
Review comment:
done
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
it doesn't handle null , this method is needed to handle that.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
+ ?
+ NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration())
+ :
+ NamespaceService.getHeartbeatNamespaceV2(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration());
+
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+
+ LOG.info("Doing healthCheck for {}", topic);
Review comment:
No reason can change to using the operator.
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +380,11 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
- public CompletableFuture<Void> healthcheckAsync() {
+ public CompletableFuture<Void> healthcheckAsync(TopicVersion topicVersion) {
WebTarget path = adminBrokers.path("health");
+ if (Objects.nonNull(topicVersion)) {
Review comment:
No reason can change to using the operator.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
+ ?
+ NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration())
+ :
+ NamespaceService.getHeartbeatNamespaceV2(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration());
+
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+
+ LOG.info("Doing healthCheck for {}", topic);
Review comment:
Will chang it
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
Ok
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
+ ?
+ NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration())
+ :
+ NamespaceService.getHeartbeatNamespaceV2(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration());
+
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+
+ LOG.info("Doing healthCheck for {}", topic);
Review comment:
Will change it
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +380,11 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
- public CompletableFuture<Void> healthcheckAsync() {
+ public CompletableFuture<Void> healthcheckAsync(TopicVersion topicVersion) {
WebTarget path = adminBrokers.path("health");
+ if (Objects.nonNull(topicVersion)) {
Review comment:
done
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
+ ?
+ NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration())
+ :
+ NamespaceService.getHeartbeatNamespaceV2(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration());
+
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+
+ LOG.info("Doing healthCheck for {}", topic);
Review comment:
done
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
done
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
ok
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
changed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10962: [WIP] Add v2 health admin endpoint
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r654031958
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -268,12 +268,12 @@
*
* @throws PulsarAdminException if the healthcheck fails.
*/
- void healthcheck() throws PulsarAdminException;
+ void healthcheck(String topicVersion) throws PulsarAdminException;
Review comment:
Maybe keep these old methods `healthcheck() and healthcheckAsync()` and add deprecated annotations? Then add the new ones. Keeps the api compatibility.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
I would make the check against v2 so the default is always the v1 format.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
+ ?
+ NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration())
+ :
+ NamespaceService.getHeartbeatNamespaceV2(
+ pulsar().getAdvertisedAddress(),
+ pulsar().getConfiguration());
+
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+
+ LOG.info("Doing healthCheck for {}", topic);
Review comment:
`Running healthcheck 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655770306
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdConsume.java
##########
@@ -414,7 +414,7 @@ private int consumeFromWebSocket(String topic) {
LOG.debug("No message to consume after waiting for 5 seconds.");
} else {
try {
- System.out.println(Base64.getDecoder().decode(msg));
+ System.out.println(Arrays.toString(Base64.getDecoder().decode(msg)));
Review comment:
why are making this change?
--
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] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655913864
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
No the default endpoint is old format, only if they choose to use the new format by using the endpoint override do they need the new ns created. It shouldn't be created by default.
--
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] aahmed-se commented on a change in pull request #10962: [Broker] Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655913864
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
No the default endpoint is using the old format, only if they choose to use the new format by using the endpoint override do they need to use the new namespace. It shouldn't be created by default.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
added 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655801162
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
We already have a new version of the method that does that.
```
public static String getHeartbeatNamespaceV2(String host, ServiceConfiguration config) {
Integer port = null;
if (config.getWebServicePort().isPresent()) {
port = config.getWebServicePort().get();
} else if (config.getWebServicePortTls().isPresent()) {
port = config.getWebServicePortTls().get();
}
return String.format(HEARTBEAT_NAMESPACE_FMT_V2, host, port);
}
```
--
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] cckellogg commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655724390
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/TopicVersion.java
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.admin;
Review comment:
Should we move this to this package `org.apache.pulsar.common.naming` and call it Version for NamingVersion?
--
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] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655768064
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
we create the v1 namespace by default
--
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] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655738605
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/TopicVersion.java
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.admin;
Review comment:
Moved it , still kept the name TopicVersion it's more clear.
--
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] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655786267
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
that's the present behaviour. it's a legacy decision.
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
##########
@@ -282,4 +282,5 @@ public static String getReasonFromServer(WebApplicationException e) {
}
}
}
+
Review comment:
removed
--
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] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655798215
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
The namespace for v2 health check topic should automatically be created. It is unreasonable to ask a user to manually create the ns of the health check 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655801162
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
We already have a new version of the method that does that.
``` public static String getHeartbeatNamespaceV2(String host, ServiceConfiguration config) {
Integer port = null;
if (config.getWebServicePort().isPresent()) {
port = config.getWebServicePort().get();
} else if (config.getWebServicePortTls().isPresent()) {
port = config.getWebServicePortTls().get();
}
return String.format(HEARTBEAT_NAMESPACE_FMT_V2, host, port);
}
```
--
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] jerrypeng commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655765724
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
Why does the v2 version fail but the v1 version passes without having to explicitly create the namespace the health check topic will be in?
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
##########
@@ -282,4 +282,5 @@ public static String getReasonFromServer(WebApplicationException e) {
}
}
}
+
Review comment:
nit unnecessary new line
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdConsume.java
##########
@@ -414,7 +414,7 @@ private int consumeFromWebSocket(String topic) {
LOG.debug("No message to consume after waiting for 5 seconds.");
} else {
try {
- System.out.println(Base64.getDecoder().decode(msg));
+ System.out.println(Arrays.toString(Base64.getDecoder().decode(msg)));
Review comment:
why are making this change?
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
where are we creating that?
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import com.google.common.collect.Sets;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.common.naming.TopicVersion;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.net.URL;
+
+@Test(groups = "broker")
+@Slf4j
+public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
+
+ @BeforeMethod
+ @Override
+ public void setup() throws Exception {
+ resetConfig();
+ super.internalSetup();
+ admin.clusters().createCluster("test",
+ ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
+ TenantInfoImpl tenantInfo = new TenantInfoImpl(
+ Sets.newHashSet("role1", "role2"), Sets.newHashSet("test"));
+ admin.tenants().createTenant("pulsar", tenantInfo);
+ admin.namespaces().createNamespace("pulsar/system", Sets.newHashSet("test"));
+ admin.tenants().createTenant("public", tenantInfo);
+ admin.namespaces().createNamespace("public/default", Sets.newHashSet("test"));
+ }
+
+ @AfterMethod(alwaysRun = true)
+ @Override
+ public void cleanup() throws Exception {
+ super.internalCleanup();
+ }
+
+ @Test
+ public void testHealthCheckup() throws Exception {
+ admin.brokers().healthcheck();
+ }
+
+ @Test
+ public void testHealthCheckupV1() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V1);
+ }
+
+ @Test(expectedExceptions = PulsarAdminException.class)
+ public void testHealthCheckupV2Error() throws Exception {
+ admin.brokers().healthcheck(TopicVersion.V2);
Review comment:
The namespace for v2 health check topic should automatically be created. It is unreasonable to ask a user to manually create the ns of the health check topic.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
You also need to update the static variable `HEARTBEAT_NAMESPACE_PATTERN` and the method `checkHeartbeatNamespace` to support v2 version of the topic
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -142,6 +142,7 @@
public static final Pattern HEARTBEAT_NAMESPACE_PATTERN = Pattern.compile("pulsar/[^/]+/([^:]+:\\d+)");
public static final Pattern SLA_NAMESPACE_PATTERN = Pattern.compile(SLA_NAMESPACE_PROPERTY + "/[^/]+/([^:]+:\\d+)");
public static final String HEARTBEAT_NAMESPACE_FMT = "pulsar/%s/%s:%s";
+ public static final String HEARTBEAT_NAMESPACE_FMT_V2 = "pulsar/%s:%s";
Review comment:
@aahmed-se I am talking about `checkHeartbeatNamespace` not `getHeartbeatNamespace`
--
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] rdhabalia commented on a change in pull request #10962: [WIP] Add v2 health admin endpoint
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r653920532
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,19 +292,19 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
+ private void healthCheck(AsyncResponse asyncResponse,
+ TopicVersion topicVersion) throws PulsarServerException {
+
validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
+ String topic;
+
+ if (topicVersion == TopicVersion.V1) {
+ String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ } else {
+ topic = ""; // TODO
Review comment:
please remove TODO
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,31 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/healthv2")
Review comment:
if you have different behavior of endpoint based on topic name then
v2 endpoint should go under: `org.apache.pulsar.broker.admin.v2.Brokers` and v1 should go under: `org.apache.pulsar.broker.admin.v1.Brokers`
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,19 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/health")
Review comment:
shouldn't this move to v1-brokers.?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,19 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/health")
Review comment:
or we can use just a query-param with topic format, if it doesn't exist then default behavior , if it does then it formats topic based on the format. that might be simple and clean change?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +387,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
Review comment:
I guess, we can avoid separate `healthCheck` method and can achieve with one line code change.
```
String namespace = (topicVersion == TopicVersion.V2) ? NamespaceService.getHeartbeatNamespaceV2(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration()) : NamespaceService.getHeartbeatNamespace(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
:
// and then same logic..
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,30 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic = null;
+ PulsarClient client = null;
+ try {
+ validateSuperUserAccess();
+
+ if (topicVersion == TopicVersion.V1) {
+ String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ } else {
+ LOG.info("Using healthCheck with V2 topic name");
Review comment:
I guess you can just add generic info log with topic name once you conclude topic eg:
`LOG.info("Doing healthCheck for {}", topic);`
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBrokers.java
##########
@@ -127,9 +128,19 @@ void run() throws Exception {
@Parameters(commandDescription = "Run a health check against the broker")
private class HealthcheckCmd extends CliCommand {
+ @Parameter(names = "--topic-version", description = "topic name version [V1,V2] V1 is default")
+ private String topicVersion;
Review comment:
make it TopicVersion enum instead string.
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBrokers.java
##########
@@ -127,9 +128,19 @@ void run() throws Exception {
@Parameters(commandDescription = "Run a health check against the broker")
private class HealthcheckCmd extends CliCommand {
+ @Parameter(names = "--topic-version", description = "topic name version [V1,V2] V1 is default")
+ private String topicVersion;
+
@Override
void run() throws Exception {
- getAdmin().brokers().healthcheck();
+ if (Objects.isNull(topicVersion) || topicVersion.equalsIgnoreCase("V1")) {
+ getAdmin().brokers().healthcheck("V1");
Review comment:
if we take enum as an input argument then we don't have to validate. we can just call
`getAdmin().brokers().healthcheck(topicVersion);`
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +368,31 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
+ @Deprecated
public CompletableFuture<Void> healthcheckAsync() {
+ return healthcheckAsync("V1");
Review comment:
same here
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +368,31 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
+ @Deprecated
public CompletableFuture<Void> healthcheckAsync() {
+ return healthcheckAsync("V1");
+ }
+
+ @Override
+ public void healthcheck(String topicVersion) throws PulsarAdminException {
Review comment:
keep input type`TopicVersion` instead `String`
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -352,9 +353,10 @@ public void failed(Throwable throwable) {
}
@Override
+ @Deprecated
public void healthcheck() throws PulsarAdminException {
try {
- healthcheckAsync().get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
+ healthcheckAsync("V1").get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
Review comment:
query param is optional and if param is null then it should consider V1 by default. so, we should not change the behavior and let it be empty which can validate that the existing behavior is working.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
we don't need any extra conditional statements anymore because protected-healthcheck method already handles topic-version type. we should remove this method and make protected-healthcheck method main public-method now because it handles all scenarios.
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +380,11 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
- public CompletableFuture<Void> healthcheckAsync() {
+ public CompletableFuture<Void> healthcheckAsync(TopicVersion topicVersion) {
WebTarget path = adminBrokers.path("health");
+ if (Objects.nonNull(topicVersion)) {
Review comment:
any reason by just not checking `topicVersion != null`, and avoiding extra method call.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
here we are not doing any check based on topic name unlike topic/namespace. therefore, adding just a query param should be fine and cleaner approach.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
sorry I read @cckellogg comment incorrectly. Yes, we should check against v2
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
it already handles null if you derive namespace
```
String namespace = (topicVersion == TopicVersion.V2) ? NamespaceService.getHeartbeatNamespaceV2( pulsar().getAdvertisedAddress(), pulsar().getConfiguration()) : NamespaceService.getHeartbeatNamespace( pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
```
--
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] aahmed-se commented on a change in pull request #10962: Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r655774547
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdConsume.java
##########
@@ -414,7 +414,7 @@ private int consumeFromWebSocket(String topic) {
LOG.debug("No message to consume after waiting for 5 seconds.");
} else {
try {
- System.out.println(Base64.getDecoder().decode(msg));
+ System.out.println(Arrays.toString(Base64.getDecoder().decode(msg)));
Review comment:
to print out the string not the byte address.
--
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] cckellogg commented on a change in pull request #10962: [Broker] Support new topic format for broker admin healthcheck endpoint
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r661795447
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -298,13 +302,35 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 404, message = "Cluster doesn't exist"),
@ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
Review comment:
we should call this `version` or `topicVersion` to be consistent with query param naming in the rest of the code base
--
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