[
https://issues.apache.org/jira/browse/KNOX-2955?focusedWorklogId=879460&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-879460
]
ASF GitHub Bot logged work on KNOX-2955:
----------------------------------------
Author: ASF GitHub Bot
Created on: 08/Sep/23 12:11
Start Date: 08/Sep/23 12:11
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #792:
URL: https://github.com/apache/knox/pull/792#discussion_r1319778438
##########
gateway-server/src/main/java/org/apache/knox/gateway/GatewayMessages.java:
##########
@@ -785,4 +784,20 @@ void failedToDiscoverClusterServices(String clusterName,
String topologyName,
@Message(level = MessageLevel.DEBUG,
text = "Request {0} is wrapped to url encoded form request.")
void wrappingRequestToUrlEncodedFormRequest(String requestURI);
+
+ @Message(level = MessageLevel.INFO,
+ text = "Checking gateway status. Deployed topologies: {0}. Waiting
for: {1}")
+ void checkingGatewayStatus(Set<String> deployedTopologies, Set<String>
missingTopologies);
+
+ @Message(level = MessageLevel.INFO,
+ text = "Checking gateway status. No topologies to check")
+ void checkingGatewayStatus();
Review Comment:
I'd use a more explicit log method name here.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/GatewayStatusService.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.knox.gateway.services.topology.impl;
+
+import java.io.File;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.commons.io.FilenameUtils;
+import org.apache.knox.gateway.GatewayMessages;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+
+public class GatewayStatusService implements Service {
+ private static final GatewayMessages LOG =
MessagesFactory.get(GatewayMessages.class);
+ private final Set<String> deployedTopologies = new HashSet<>();
+ private Set<String> topologyNamesToCheck = new HashSet<>();
+ private GatewayConfig config;
+
+ public synchronized void onTopologyReady(String topologyName) {
+ deployedTopologies.add(topologyName);
+ }
+
+ public synchronized boolean status() {
+ if (topologyNamesToCheck.isEmpty()) {
+ LOG.checkingGatewayStatus();
+ return false;
+ }
+ Set<String> missing = new HashSet<>(topologyNamesToCheck);
+ missing.removeAll(deployedTopologies);
+ LOG.checkingGatewayStatus(deployedTopologies, missing);
+ return missing.isEmpty();
+ }
+
+ /**
+ * The list of topologies (which will be used to check the gateway status.)
+ * are either coming from the config or collected automatically.
+ * In the later case this should be called at startup, after the hadoop xml
resource parser
+ * already generated the descriptors from the hxr
+ */
+ public synchronized void initTopologiesToCheck() {
+ Set<String> healthCheckTopologies = config.getHealthCheckTopologies();
+ if (healthCheckTopologies.isEmpty()) {
+ topologyNamesToCheck = collectTopologies(config);
+ } else {
+ topologyNamesToCheck = healthCheckTopologies;
+ }
+ LOG.startingStatusMonitor(topologyNamesToCheck);
+ }
+
+ private Set<String> collectTopologies(GatewayConfig config) {
+ Set<String> result = new HashSet<>();
+ collectFiles(result, config.getGatewayTopologyDir(), ".xml");
+ collectFiles(result, config.getGatewayDescriptorsDir(), ".json");
+ LOG.collectedTopologiesForHealthCheck(result);
+ return result;
+ }
+
+ private void collectFiles(Set<String> result, String directory, String
extension) {
+ File[] files = new File(directory).getAbsoluteFile()
+ .listFiles((dir, name) ->
name.toLowerCase(Locale.ROOT).endsWith(extension));
+ if (files != null) {
+ for (File file : files) {
+ result.add(FilenameUtils.getBaseName(file.getName()));
+ }
+ }
+ }
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws
ServiceLifecycleException {
Review Comment:
I'd add a comment here as to why `initTopologiesToCheck` cannot be part of
the regular `init` method (resources are not yet ready at this point). To make
it clear to future readers.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/factory/GatewayStatusCheckerFactory.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.knox.gateway.services.factory;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.unmodifiableList;
+
+import java.util.Collection;
+import java.util.Map;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.ServiceType;
+import org.apache.knox.gateway.services.topology.impl.GatewayStatusService;
+
+public class GatewayStatusCheckerFactory extends AbstractServiceFactory {
Review Comment:
This should be renamed to `GatewayStatusServiceFactory`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/GatewayMessages.java:
##########
@@ -785,4 +784,20 @@ void failedToDiscoverClusterServices(String clusterName,
String topologyName,
@Message(level = MessageLevel.DEBUG,
text = "Request {0} is wrapped to url encoded form request.")
void wrappingRequestToUrlEncodedFormRequest(String requestURI);
+
+ @Message(level = MessageLevel.INFO,
+ text = "Checking gateway status. Deployed topologies: {0}. Waiting
for: {1}")
+ void checkingGatewayStatus(Set<String> deployedTopologies, Set<String>
missingTopologies);
+
+ @Message(level = MessageLevel.INFO,
+ text = "Checking gateway status. No topologies to check")
+ void checkingGatewayStatus();
+
+ @Message(level = MessageLevel.INFO,
+ text = "Collected topologies for health check: {0}")
+ void collectedTopologiesForHealthCheck(Set<String> result);
+
+ @Message(level = MessageLevel.INFO,
+ text = "Starting gateway status checker service. Topologies to
check: {0}")
Review Comment:
Maybe removing the word `checker`?
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/topology/impl/GatewayStatusServiceTest.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.knox.gateway.services.topology.impl;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.easymock.EasyMock;
+import org.junit.Test;
+
+public class GatewayStatusServiceTest {
+
+ @Test
+ public void testReadyStatus() throws Exception {
+ GatewayStatusService statusChecker = new GatewayStatusService();
Review Comment:
`statusChecker` -> `statusService`
##########
gateway-service-health/src/main/java/org/apache/knox/gateway/service/health/PingResource.java:
##########
@@ -81,7 +87,26 @@ private Response getPingResponse() {
}
String getPingContent() {
- return CONTENT;
+ return OK;
+ }
+
+ @GET
+ @Produces({TEXT_PLAIN})
+ @Path("gateway-status")
+ public Response status() {
+ response.setStatus(HttpServletResponse.SC_OK);
Review Comment:
The `response` field is always set to the same here and in `getPingResponse`
too. You may want to refactor this out to an `init` method and call it only
once (e.g. using the `PostConstruct` annotation as we do in other resource
classes).
##########
gateway-service-health/src/main/java/org/apache/knox/gateway/service/health/PingResource.java:
##########
@@ -81,7 +87,26 @@ private Response getPingResponse() {
}
String getPingContent() {
- return CONTENT;
+ return OK;
+ }
+
+ @GET
+ @Produces({TEXT_PLAIN})
+ @Path("gateway-status")
+ public Response status() {
+ response.setStatus(HttpServletResponse.SC_OK);
+ response.setHeader(CACHE_CONTROL, NO_CACHE);
+ response.setContentType(CONTENT_TYPE);
+ GatewayServices services = (GatewayServices) request.getServletContext()
+ .getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
+ GatewayStatusService statusChecker =
services.getService(ServiceType.GATEWAY_STATUS_SERVICE);
Review Comment:
`statusChecker` -> `statusService`
Issue Time Tracking
-------------------
Worklog Id: (was: 879460)
Remaining Estimate: 0h
Time Spent: 10m
> Knox Readiness Awareness and Notification
> -----------------------------------------
>
> Key: KNOX-2955
> URL: https://issues.apache.org/jira/browse/KNOX-2955
> Project: Apache Knox
> Issue Type: Bug
> Reporter: Attila Magyar
> Assignee: Attila Magyar
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently, Knox is unable to accurately report its readiness to handle
> requests (e.g., all topology deployments have completed).
> Knox needs a more reliable means by which to know that all of the topologies
> have been completely deployed before reporting that it is "ready".
> Knox also needs a new built-in endpoint for querying this readiness, which
> does not require authentication.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
