jackjlli commented on code in PR #13742:
URL: https://github.com/apache/pinot/pull/13742#discussion_r1715967855
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##########
@@ -112,15 +139,22 @@ public Map<ServerRoutingInstance, ServerResponse>
getFinalResponses()
public String getServerStats() {
StringBuilder stringBuilder = new StringBuilder(
"(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs)");
- for (Map.Entry<ServerRoutingInstance, ServerResponse> entry :
_responseMap.entrySet()) {
-
stringBuilder.append(';').append(entry.getKey().getShortName()).append('=').append(entry.getValue().toString());
+ for (Map.Entry<ServerRoutingInstance, ConcurrentHashMap<Integer,
ServerResponse>> serverResponses
+ : _responses.entrySet()) {
+ for (Map.Entry<Integer, ServerResponse> responsePair :
serverResponses.getValue().entrySet()) {
+ ServerRoutingInstance serverRoutingInstance = serverResponses.getKey();
+
stringBuilder.append(';').append(serverRoutingInstance.getShortName()).append('=')
+ .append(responsePair.getValue().toString());
+ }
}
return stringBuilder.toString();
}
@Override
public long getServerResponseDelayMs(ServerRoutingInstance
serverRoutingInstance) {
- return _responseMap.get(serverRoutingInstance).getResponseDelayMs();
+ // TODO(egalpin): How to get query hash here?
+ return -1L;
+// return _responseMap.get(serverRoutingInstance).getResponseDelayMs();
Review Comment:
This needs to be uncommented, right?
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryResponse.java:
##########
@@ -49,12 +50,12 @@ enum Status {
/**
* Returns the current server responses without blocking.
*/
- Map<ServerRoutingInstance, ServerResponse> getCurrentResponses();
+ Map<ServerRoutingInstance, List<ServerResponse>> getCurrentResponses();
Review Comment:
I don't think the code change in this PR is backward compatible. We
definitely need to keep the existing signatures as is. Can we introduce new
signatures and mark the existing ones `Deprecated`, so that the old signatures
can be cleaned up in the next official release?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -771,15 +756,14 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
// - Compile time function invocation
// - Literal only queries
// - Any rewrites
- if (pinotQuery.isExplain()) {
+// if (pinotQuery.isExplain()) {
Review Comment:
nit: why is it commented out?
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerRoutingInstance.java:
##########
@@ -103,16 +93,16 @@ public boolean equals(Object o) {
return false;
}
ServerRoutingInstance that = (ServerRoutingInstance) o;
- // NOTE: Only check hostname, port and tableType for performance concern
because they can identify a routing
+ // NOTE: Only check hostname and port for performance concern because they
can identify a routing
// instance within the same query
- return _hostname.equals(that._hostname) && _port == that._port &&
_tableType == that._tableType;
+ return _hostname.equals(that._hostname) && _port == that._port;
}
@Override
public int hashCode() {
- // NOTE: Only check hostname, port and tableType for performance concern
because they can identify a routing
+ // NOTE: Only check hostname and port for performance concern because they
can identify a routing
// instance within the same query
- return 31 * 31 * _hostname.hashCode() + 31 * Integer.hashCode(_port) +
_tableType.hashCode();
+ return 31 * _hostname.hashCode() + Integer.hashCode(_port);
Review Comment:
This will make the hashcode inconsistent between two different versions of
broker and servers. Can you double check if this has any side effects?
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerRoutingInstance.java:
##########
@@ -23,43 +23,36 @@
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import javax.annotation.concurrent.ThreadSafe;
-import org.apache.pinot.spi.config.table.TableType;
import org.apache.pinot.spi.utils.CommonConstants.Helix;
/**
* The {@code ServerRoutingInstance} class represents the routing target
instance which contains the information of
- * hostname, port, and table type it serves.
- * <p>Different table types on same host and port are counted as different
instances. Therefore, one single Pinot Server
- * might be treated as two different routing target instances based on the
types of table it serves.
+ * hostname and port.
*/
@ThreadSafe
public final class ServerRoutingInstance {
- private static final String SHORT_OFFLINE_SUFFIX = "_O";
Review Comment:
I think with this change we kind of lose the way of knowing how many servers
get queried/respond for each table type. I understand that the number of
queries being hit to the same server can be reduced, but is it possible to
include this table type information in the request and response, so that we
don't lose this stat?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
