imply-cheddar commented on code in PR #12867:
URL: https://github.com/apache/druid/pull/12867#discussion_r949451777
##########
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:
Just because it is "okay" to add a thing, forcing anybody that happens to
implement an interface to implement a new method adds friction to moving
forward. It's "okay" to provide for *some* way to make changes and move things
forward, not to say that it's okay to force people to make simple random
changes to keep up with versions. Always make it as simple as possible for
someone to bring their extension forward a version, if it's relatively easy to
make it so that nobody has to change anything and they get good behavior, then
that should be done.
Now, if the default implementation doesn't provide good behavior and instead
causes bad things to happen, then people should be forced to implement it. In
this case, it seems like a default is good?
--
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]
