kfaraz commented on code in PR #18709:
URL: https://github.com/apache/druid/pull/18709#discussion_r2485301506
##########
server/src/main/java/org/apache/druid/server/metrics/DataSourceTaskIdHolder.java:
##########
@@ -46,11 +48,13 @@ public class DataSourceTaskIdHolder
@Inject(optional = true)
BroadcastDatasourceLoadingSpec broadcastDatasourceLoadingSpec =
BroadcastDatasourceLoadingSpec.ALL;
+ @Nullable
Review Comment:
Please add a short javadoc on these getters mentioning that these are
non-null only for CliPeon servers.
In a follow up PR, we should also fix up the way that
`DatasourceTaskIdHolder` is wired and bind that class itself only for peons.
There is no meaning in having this instance bound but contain null values for
datasource and task ID.
##########
server/src/main/java/org/apache/druid/server/metrics/GroupByStatsMonitor.java:
##########
@@ -41,21 +43,28 @@ public class GroupByStatsMonitor extends AbstractMonitor
{
private final GroupByStatsProvider groupByStatsProvider;
private final BlockingPool<ByteBuffer> mergeBufferPool;
+ private final Map<String, String[]> dimensions;
@Inject
public GroupByStatsMonitor(
GroupByStatsProvider groupByStatsProvider,
- @Merging BlockingPool<ByteBuffer> mergeBufferPool
+ @Merging BlockingPool<ByteBuffer> mergeBufferPool,
+ DataSourceTaskIdHolder dataSourceTaskIdHolder
Review Comment:
Please pass a dimensions map here instead of this holder since the holder
class has meaning only for peons.
##########
server/src/main/java/org/apache/druid/server/metrics/GroupByStatsMonitor.java:
##########
@@ -41,21 +43,28 @@ public class GroupByStatsMonitor extends AbstractMonitor
{
private final GroupByStatsProvider groupByStatsProvider;
private final BlockingPool<ByteBuffer> mergeBufferPool;
+ private final Map<String, String[]> dimensions;
Review Comment:
Maybe rename to `serviceDimensions`?
In fact, I wonder if it wouldn't be cleaner to just bind the datasource and
taskID as `@ExtraServiceDimensions` so that they are bound in `EmitterModule`
and are emitted for all metrics emitted by a peon.
--
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]
