gortiz commented on code in PR #18602:
URL: https://github.com/apache/pinot/pull/18602#discussion_r3317455929
##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java:
##########
@@ -649,4 +652,17 @@ public void setMaterializedViewQueried(@Nullable String
materializedViewQueried)
public String getMaterializedViewQueried() {
return _materializedViewQueried;
}
+
+ @JsonProperty("serverStats")
+ public void setServerStats(@Nullable String serverStats) {
+ _serverStats = serverStats;
+ }
+
+ @Nullable
+ @JsonProperty("serverStats")
+ @JsonInclude(JsonInclude.Include.NON_NULL)
+ @Override
+ public String getServerStats() {
+ return _serverStats;
+ }
Review Comment:
I don't think stats should be a string. Couldn't it be a JSON like we do in
MSE with stageStats?
##########
pinot-common/src/test/java/org/apache/pinot/common/response/broker/BrokerResponseNativeTest.java:
##########
@@ -85,4 +87,43 @@ public void testMaterializedViewQueriedAbsentWhenNull()
Assert.assertFalse(json.contains("materializedViewQueried"),
"Null materializedViewQueried should be suppressed by
@JsonInclude(NON_NULL)");
}
+
+ @Test
+ public void testServerStatsRoundTrip()
+ throws IOException {
+ // BaseSingleStageBrokerRequestHandler#handleRequest copies the
AsyncQueryResponse-format
+ // per-server stats string onto the response after scatter; downstream
subscribers
+ // (onQueryCompletion hooks and JSON-API clients) read it back from here.
Verify the field
+ // round-trips through Jackson serialization unchanged.
+ BrokerResponseNative expected = new BrokerResponseNative();
+ String stats =
+
"Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs;"
+ + "pinot-server-0_O=0,1,7571,0,0;pinot-server-1_O=0,1,7574,0,0";
+ expected.setServerStats(stats);
Review Comment:
This is an example of why stats shouldn't be a string. It is super difficult
to parse them and very easy to make a backward incompatible change
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -449,4 +449,19 @@ default long getRealtimeTotalMemAllocatedBytes() {
default String getMaterializedViewQueried() {
return null;
}
+
+ /// Get the per-server stats string captured during scatter for this query,
or `null` if not
+ /// populated. For single-stage queries served by the Netty
(single-connection) transport the
+ /// value is the formatted string built by
+ /// `org.apache.pinot.core.transport.AsyncQueryResponse#getServerStats()`
(one entry per scattered
+ /// server with submit/response/deserialization timings and response size).
Other transports
+ /// (e.g. gRPC) and multi-stage queries leave this `null` because OSS does
not currently compute a
+ /// per-server stats string on those paths. The default returns `null` so
impls that do not
+ /// track per-server stats (e.g. MSE response paths) need no explicit
override; the matching
+ /// *setter* is deliberately not defaulted here, mirroring the asymmetry
used by
+ /// [#getMaterializedViewQueried] above.
+ @Nullable
+ default String getServerStats() {
+ return null;
+ }
Review Comment:
If this is only used in SSE... do we actually need it on the interface? It
would be safer to introduce this getter only in the SSE response class. We do
the same for some MSE only getters
--
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]
