rohangarg commented on code in PR #12867:
URL: https://github.com/apache/druid/pull/12867#discussion_r949494970
##########
processing/src/main/java/org/apache/druid/query/GenericQueryMetricsFactory.java:
##########
@@ -52,11 +50,7 @@
/**
* Creates a {@link QueryMetrics} which doesn't have predefined QueryMetrics
subclass. This method is used
* by the router to build a {@link QueryMetrics} for SQL queries. It is
needed since at router, there is no native
- * query linked to a SQL query. By default, it returns null to maintain
backward compatibility.
+ * query linked to a SQL query.
*/
- @Nullable
- default QueryMetrics<Query<?>> makeMetrics()
- {
- return null;
- }
+ QueryMetrics<Query<?>> makeMetrics();
Review Comment:
Yes, I totally agree with your sentiment regarding SPI compat being more of
a judgement call rather than a technicality. I started out with a default
implementation and then removed it due to the above rationale. But I missed
adding the full explanation in the previous comment.
After this discussion I was also thinking if we could change
`makeMetrics(Query)` to `makeMetrics(@Nullable Query)` and then make the
`makeMetrics` default implementation as `makeMetrics(null)`. But again, with
that I think that might impact the semantics of `makeMetrics(Query)` to expect
a non-null `Query`.
I'll again think if any other way is possible to avoid incompatbility and
update if I find one.
--
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]
