From c465389b7796fcfd218500976f9611fba1047dc4 Mon Sep 17 00:00:00 2001 From: tibrewalpratik Date: Tue, 12 Sep 2023 15:02:09 +0530 Subject: [PATCH] address comments 2 --- .../broker/helix/BaseBrokerStarter.java | 4 ++-- .../BaseBrokerRequestHandler.java | 10 ++++---- .../GrpcBrokerRequestHandler.java | 2 +- .../MultiStageBrokerRequestHandler.java | 2 +- .../SingleConnectionBrokerRequestHandler.java | 2 +- .../LiteralOnlyBrokerRequestTest.java | 2 +- .../MultiStageBrokerRequestHandlerTest.java | 2 +- .../query}/BrokerQueryEventInfo.java | 24 +++++++++---------- .../query}/BrokerQueryEventListener.java | 6 ++++- .../query}/NoOpBrokerQueryEventListener.java | 10 +++++++- .../PinotBrokerQueryEventListenerUtils.java | 5 ++-- .../pinot/spi/utils/CommonConstants.java | 2 +- 12 files changed, 43 insertions(+), 28 deletions(-) rename pinot-spi/src/main/java/org/apache/pinot/spi/{queryeventlistener => eventlistener/query}/BrokerQueryEventInfo.java (97%) rename pinot-spi/src/main/java/org/apache/pinot/spi/{queryeventlistener => eventlistener/query}/BrokerQueryEventListener.java (84%) rename pinot-spi/src/main/java/org/apache/pinot/spi/{queryeventlistener => eventlistener/query}/NoOpBrokerQueryEventListener.java (82%) rename pinot-spi/src/main/java/org/apache/pinot/spi/{queryeventlistener => eventlistener/query}/PinotBrokerQueryEventListenerUtils.java (96%) diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java index 27494c3a16e..9185ecc13d4 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java +++ b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java @@ -70,10 +70,10 @@ import org.apache.pinot.core.util.ListenerConfigUtil; import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider; import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener; +import org.apache.pinot.spi.eventlistener.query.PinotBrokerQueryEventListenerUtils; import org.apache.pinot.spi.metrics.PinotMetricUtils; import org.apache.pinot.spi.metrics.PinotMetricsRegistry; -import org.apache.pinot.spi.queryeventlistener.BrokerQueryEventListener; -import org.apache.pinot.spi.queryeventlistener.PinotBrokerQueryEventListenerUtils; import org.apache.pinot.spi.services.ServiceRole; import org.apache.pinot.spi.services.ServiceStartable; import org.apache.pinot.spi.trace.Tracing; diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java index 045958ef3d9..36e0e9a19c0 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java +++ b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java @@ -89,9 +89,9 @@ import org.apache.pinot.spi.config.table.TableType; import org.apache.pinot.spi.data.Schema; import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventInfo; +import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener; import org.apache.pinot.spi.exception.BadQueryRequestException; -import org.apache.pinot.spi.queryeventlistener.BrokerQueryEventInfo; -import org.apache.pinot.spi.queryeventlistener.BrokerQueryEventListener; import org.apache.pinot.spi.trace.RequestContext; import org.apache.pinot.spi.trace.Tracing; import org.apache.pinot.spi.utils.BytesUtils; @@ -246,6 +246,9 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable SqlNodeAndOption throws Exception { requestContext.setRequestArrivalTimeMillis(System.currentTimeMillis()); + long requestId = _brokerIdGenerator.get(); + requestContext.setRequestId(requestId); + // First-stage access control to prevent unauthenticated requests from using up resources. Secondary table-level // check comes later. boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity); @@ -256,10 +259,9 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable SqlNodeAndOption throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); } - long requestId = _brokerIdGenerator.get(); - requestContext.setRequestId(requestId); JsonNode sql = request.get(Broker.Request.SQL); if (sql == null) { + requestContext.setErrorCode(QueryException.BROKER_REQUEST_SEND_ERROR_CODE); _brokerQueryEventListener.onQueryCompletion(new BrokerQueryEventInfo(requestContext)); throw new BadQueryRequestException("Failed to find 'sql' in the request: " + request); } diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/GrpcBrokerRequestHandler.java b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/GrpcBrokerRequestHandler.java index 0e5d73d703d..b2e36d16eff 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/GrpcBrokerRequestHandler.java +++ b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/GrpcBrokerRequestHandler.java @@ -42,7 +42,7 @@ import org.apache.pinot.core.transport.ServerRoutingInstance; import org.apache.pinot.spi.config.table.TableType; import org.apache.pinot.spi.env.PinotConfiguration; -import org.apache.pinot.spi.queryeventlistener.BrokerQueryEventListener; +import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener; import org.apache.pinot.spi.trace.RequestContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java index 211f0a20b8e..46f6200bb03 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java +++ b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java @@ -62,7 +62,7 @@ import org.apache.pinot.query.type.TypeFactory; import org.apache.pinot.query.type.TypeSystem; import org.apache.pinot.spi.env.PinotConfiguration; -import org.apache.pinot.spi.queryeventlistener.BrokerQueryEventListener; +import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener; import org.apache.pinot.spi.trace.RequestContext; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.spi.utils.builder.TableNameBuilder; diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java index 592f82a8512..3ad86484426 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java +++ b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java @@ -50,7 +50,7 @@ import org.apache.pinot.core.transport.ServerRoutingInstance; import org.apache.pinot.core.transport.server.routing.stats.ServerRoutingStatsManager; import org.apache.pinot.spi.env.PinotConfiguration; -import org.apache.pinot.spi.queryeventlistener.BrokerQueryEventListener; +import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener; import org.apache.pinot.spi.trace.RequestContext; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.spi.utils.builder.TableNameBuilder; diff --git a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java index 4f7183b7745..46ce5a4ce9e 100644 --- a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java +++ b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java @@ -31,8 +31,8 @@ import org.apache.pinot.common.utils.DataSchema; import org.apache.pinot.core.transport.server.routing.stats.ServerRoutingStatsManager; import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.eventlistener.query.PinotBrokerQueryEventListenerUtils; import org.apache.pinot.spi.metrics.PinotMetricUtils; -import org.apache.pinot.spi.queryeventlistener.PinotBrokerQueryEventListenerUtils; import org.apache.pinot.spi.trace.RequestContext; import org.apache.pinot.spi.trace.Tracing; import org.apache.pinot.spi.utils.BytesUtils; diff --git a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandlerTest.java b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandlerTest.java index 0a8a06c1baa..fa0119d77c9 100644 --- a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandlerTest.java +++ b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandlerTest.java @@ -29,7 +29,7 @@ import org.apache.pinot.common.config.provider.TableCache; import org.apache.pinot.common.metrics.BrokerMetrics; import org.apache.pinot.spi.env.PinotConfiguration; -import org.apache.pinot.spi.queryeventlistener.PinotBrokerQueryEventListenerUtils; +import org.apache.pinot.spi.eventlistener.query.PinotBrokerQueryEventListenerUtils; import org.apache.pinot.spi.trace.DefaultRequestContext; import org.apache.pinot.spi.trace.RequestContext; import org.apache.pinot.spi.utils.CommonConstants; diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/BrokerQueryEventInfo.java b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/BrokerQueryEventInfo.java similarity index 97% rename from pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/BrokerQueryEventInfo.java rename to pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/BrokerQueryEventInfo.java index 6d7445288bf..79544433d33 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/BrokerQueryEventInfo.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/BrokerQueryEventInfo.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.pinot.spi.queryeventlistener; +package org.apache.pinot.spi.eventlistener.query; import java.util.ArrayList; import java.util.Collections; @@ -25,15 +25,15 @@ public class BrokerQueryEventInfo { - private String _requestId; - private String _brokerId; - private String _query; - private String _queryStatus; - private String _failureJson; - private int _errorCode; - - private long _requestArrivalTimeMillis; - private int _numServersQueried; + private String _requestId = ""; + private String _brokerId = ""; + private String _query = ""; + private String _queryStatus = ""; + private String _failureJson = ""; + private int _errorCode = -1; + + private long _requestArrivalTimeMillis = 0L; + private int _numServersQueried = 0; private int _numServersResponded = 0; private long _numDocsScanned = 0L; private long _numEntriesScannedInFilter = 0L; @@ -67,8 +67,8 @@ public class BrokerQueryEventInfo { private long _explainPlanNumMatchAllFilterSegments = 0L; private int _numRowsResultSet = 0; private List _tableNames = new ArrayList<>(); - private String _offlineServerTenant; - private String _realtimeServerTenant; + private String _offlineServerTenant = ""; + private String _realtimeServerTenant = ""; public BrokerQueryEventInfo() { } diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/BrokerQueryEventListener.java b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/BrokerQueryEventListener.java similarity index 84% rename from pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/BrokerQueryEventListener.java rename to pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/BrokerQueryEventListener.java index 69052e3b692..75e541ad4f8 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/BrokerQueryEventListener.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/BrokerQueryEventListener.java @@ -16,9 +16,13 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.pinot.spi.queryeventlistener; +package org.apache.pinot.spi.eventlistener.query; + +import org.apache.pinot.spi.env.PinotConfiguration; + public interface BrokerQueryEventListener { + void init(PinotConfiguration eventListenerConfiguration); void onQueryCompletion(BrokerQueryEventInfo brokerQueryEventInfo); } diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/NoOpBrokerQueryEventListener.java b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/NoOpBrokerQueryEventListener.java similarity index 82% rename from pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/NoOpBrokerQueryEventListener.java rename to pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/NoOpBrokerQueryEventListener.java index fb8fc2dc49b..e01ec5f356d 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/NoOpBrokerQueryEventListener.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/NoOpBrokerQueryEventListener.java @@ -16,10 +16,18 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.pinot.spi.queryeventlistener; +package org.apache.pinot.spi.eventlistener.query; + +import org.apache.pinot.spi.env.PinotConfiguration; + public class NoOpBrokerQueryEventListener implements BrokerQueryEventListener { + @Override + public void init(PinotConfiguration eventListenerConfiguration) { + // Not implemented method + } + @Override public void onQueryCompletion(BrokerQueryEventInfo brokerQueryEventInfo) { // Not implemented method diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/PinotBrokerQueryEventListenerUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/PinotBrokerQueryEventListenerUtils.java similarity index 96% rename from pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/PinotBrokerQueryEventListenerUtils.java rename to pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/PinotBrokerQueryEventListenerUtils.java index 174fe8b0abb..23f8277ccd0 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/queryeventlistener/PinotBrokerQueryEventListenerUtils.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/eventlistener/query/PinotBrokerQueryEventListenerUtils.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.pinot.spi.queryeventlistener; +package org.apache.pinot.spi.eventlistener.query; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -67,6 +67,7 @@ private static void initializeBrokerQueryEventListener(PinotConfiguration eventL clazzFound.ifPresent(clazz -> { try { BrokerQueryEventListener brokerQueryEventListener = (BrokerQueryEventListener) clazz.newInstance(); + brokerQueryEventListener.init(eventListenerConfiguration); registerBrokerEventListener(brokerQueryEventListener); } catch (Exception e) { LOGGER.error("Caught exception while initializing event listener registry: {}, skipping it", clazz, e); @@ -78,7 +79,7 @@ private static void initializeBrokerQueryEventListener(PinotConfiguration eventL } /** - * Registers an broker event listener. + * Registers a broker event listener. */ private static void registerBrokerEventListener(BrokerQueryEventListener brokerQueryEventListener) { LOGGER.info("Registering broker event listener : {}", brokerQueryEventListener.getClass().getName()); diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java index 988751d687e..17fc663c9b3 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java @@ -46,7 +46,7 @@ private CommonConstants() { public static final String DEFAULT_METRICS_FACTORY_CLASS_NAME = "org.apache.pinot.plugin.metrics.yammer.YammerMetricsFactory"; public static final String DEFAULT_BROKER_EVENT_LISTENER_CLASS_NAME = - "org.apache.pinot.spi.queryeventlistener.NoOpBrokerQueryEventListener"; + "org.apache.pinot.spi.eventlistener.query.NoOpBrokerQueryEventListener"; public static final String SWAGGER_AUTHORIZATION_KEY = "oauth"; public static final String CONFIG_OF_SWAGGER_RESOURCES_PATH = "META-INF/resources/webjars/swagger-ui/5.1.0/";