Jackie-Jiang commented on code in PR #13122:
URL: https://github.com/apache/pinot/pull/13122#discussion_r1605914468
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java:
##########
@@ -115,15 +112,15 @@ protected void configure() {
bind(startTime).named(BrokerAdminApiApplication.START_TIME);
}
});
- boolean enableBoundedJerseyThreadPoolExecutor =
-
brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_ENABLE_BOUNDED_JERSEY_THREADPOOL_EXECUTOR,
+ boolean enableBoundedJerseyThreadPoolExecutor = brokerConf
+
.getProperty(CommonConstants.Broker.CONFIG_OF_ENABLE_BOUNDED_JERSEY_THREADPOOL_EXECUTOR,
CommonConstants.Broker.DEFAULT_ENABLE_BOUNDED_JERSEY_THREADPOOL_EXECUTOR);
if (enableBoundedJerseyThreadPoolExecutor) {
register(buildBrokerManagedAsyncExecutorProvider(brokerConf,
brokerMetrics));
}
register(JacksonFeature.class);
- registerClasses(io.swagger.jaxrs.listing.ApiListingResource.class);
- registerClasses(io.swagger.jaxrs.listing.SwaggerSerializers.class);
+ register(SwaggerApiListingResource.class);
+ register(io.swagger.jaxrs.listing.SwaggerSerializers.class);
Review Comment:
Can we import this class? Same for other places
##########
pinot-common/src/main/java/org/apache/pinot/common/swagger/SwaggerSetupUtil.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.pinot.common.swagger;
+
+import io.swagger.jaxrs.config.BeanConfig;
+import java.net.InetAddress;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.net.UnknownHostException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.PinotStaticHttpHandler;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.glassfish.grizzly.http.server.CLStaticHttpHandler;
+import org.glassfish.grizzly.http.server.HttpServer;
+
+
+public class SwaggerSetupUtil {
Review Comment:
(convention)
```suggestion
public class SwaggerSetupUtils {
```
##########
pinot-common/src/main/java/org/apache/pinot/common/swagger/SwaggerSetupUtil.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.pinot.common.swagger;
+
+import io.swagger.jaxrs.config.BeanConfig;
+import java.net.InetAddress;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.net.UnknownHostException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.PinotStaticHttpHandler;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.glassfish.grizzly.http.server.CLStaticHttpHandler;
+import org.glassfish.grizzly.http.server.HttpServer;
+
+
+public class SwaggerSetupUtil {
+ private SwaggerSetupUtil() {
+ }
+
+ public static void setupSwagger(String componentType, String resourcePackage,
+ boolean useHttps, boolean setHost, String basePath, ClassLoader
classLoader, HttpServer httpServer) {
+ BeanConfig beanConfig = new BeanConfig();
+ String capitalizeComponentType = StringUtils.capitalize(componentType);
+ beanConfig.setTitle(String.format("Pinot %s API",
capitalizeComponentType));
+ beanConfig.setDescription(String.format("APIs for accessing Pinot %s
information", capitalizeComponentType));
+ beanConfig.setContact("https://github.com/apache/pinot";);
+ beanConfig.setVersion("1.0");
+ beanConfig.setExpandSuperTypes(false);
+ if (useHttps) {
+ beanConfig.setSchemes(new String[]{CommonConstants.HTTPS_PROTOCOL});
+ } else {
+ beanConfig.setSchemes(new String[]{CommonConstants.HTTP_PROTOCOL,
CommonConstants.HTTPS_PROTOCOL});
+ }
+ beanConfig.setBasePath(basePath);
+ beanConfig.setResourcePackage(resourcePackage);
+ beanConfig.setScan(true);
+ if (setHost) {
+ try {
+ beanConfig.setHost(InetAddress.getLocalHost().getHostName());
+ } catch (UnknownHostException e) {
+ throw new RuntimeException("Cannot get localhost name");
+ }
+ }
+
+ CLStaticHttpHandler staticHttpHandler = new
CLStaticHttpHandler(classLoader, "/api/");
+ // map both /api and /help to swagger docs. /api because it looks nice.
/help for backward compatibility
+ httpServer.getServerConfiguration().addHttpHandler(staticHttpHandler,
"/api/", "/help/");
+
+ URL swaggerDistLocation =
classLoader.getResource(CommonConstants.CONFIG_OF_SWAGGER_RESOURCES_PATH);
Review Comment:
We can move this constant into this class. This is not really a config
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java:
##########
@@ -89,8 +85,9 @@ public BrokerAdminApiApplication(BrokerRoutingManager
routingManager, BrokerRequ
_executorService =
Executors.newCachedThreadPool(new
ThreadFactoryBuilder().setNameFormat("async-task-thread-%d").build());
PoolingHttpClientConnectionManager connMgr = new
PoolingHttpClientConnectionManager();
- int timeoutMs = (int)
brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_TIMEOUT_MS,
- CommonConstants.Broker.DEFAULT_BROKER_TIMEOUT_MS);
+ int timeoutMs = (int) brokerConf
Review Comment:
(code style) Can you double check the code style setting? I tried
auto-formatting the file and got the same formatting as the original file
##########
pinot-common/src/main/java/org/apache/pinot/common/swagger/SwaggerSetupUtil.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.pinot.common.swagger;
+
+import io.swagger.jaxrs.config.BeanConfig;
+import java.net.InetAddress;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.net.UnknownHostException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.PinotStaticHttpHandler;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.glassfish.grizzly.http.server.CLStaticHttpHandler;
+import org.glassfish.grizzly.http.server.HttpServer;
+
+
+public class SwaggerSetupUtil {
+ private SwaggerSetupUtil() {
+ }
+
+ public static void setupSwagger(String componentType, String resourcePackage,
+ boolean useHttps, boolean setHost, String basePath, ClassLoader
classLoader, HttpServer httpServer) {
Review Comment:
I think we can probably always set host. I guess we just forgot to update
all places when adding it
##########
pinot-common/src/main/java/org/apache/pinot/common/swagger/SwaggerApiListingResource.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.pinot.common.swagger;
+
+import io.swagger.annotations.ApiOperation;
+import io.swagger.jaxrs.listing.BaseApiListingResource;
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletContext;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.Application;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+
+/*
+ This class is required to avoid the jersey2 warning messages regarding
servlet config constructor missing while
+ injecting the config. Please refer to Pinot issue 13047 & 5306 for more
context.
+ In this implementation, we added the ServletConfig as the class level member
instead of injecting it.
+*/
+@Path("/swagger.{type:json|yaml}")
+public class SwaggerApiListingResource extends BaseApiListingResource {
+ @Context
+ ServletContext _context;
+
+ @Context
+ ServletConfig _servletConfig;
+
+ @GET
+ @Produces({MediaType.APPLICATION_JSON, "application/yaml"})
+ @ApiOperation(value = "The swagger definition in either JSON or YAML",
hidden = true)
+ public Response getListing(
Review Comment:
(code format) I guess this should be reformatted
##########
pinot-common/src/main/java/org/apache/pinot/common/swagger/SwaggerSetupUtil.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.pinot.common.swagger;
+
+import io.swagger.jaxrs.config.BeanConfig;
+import java.net.InetAddress;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.net.UnknownHostException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.PinotStaticHttpHandler;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.glassfish.grizzly.http.server.CLStaticHttpHandler;
+import org.glassfish.grizzly.http.server.HttpServer;
+
+
+public class SwaggerSetupUtil {
+ private SwaggerSetupUtil() {
+ }
+
+ public static void setupSwagger(String componentType, String resourcePackage,
Review Comment:
I guess we can just pass in capitalized component type. It is okay to use
capitalized component type in the description
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
