kfaraz commented on code in PR #18732:
URL: https://github.com/apache/druid/pull/18732#discussion_r2570645814
##########
server/src/main/java/org/apache/druid/server/metrics/MonitorsConfig.java:
##########
@@ -81,19 +78,19 @@ public String toString()
}
/**
- * @return a map of {@code datasource} and {@code taskId} dimensions if
provided; otherwise, returns an empty map.
- * When {@code taskId} is provided, both {@link DruidMetrics#ID} and {@link
DruidMetrics#TASK_ID} dimensions are added
- * to the map for backward compatibility. {@link DruidMetrics#ID} is
deprecated because it's ambiguous and will be
- * removed in a future release.
+ * @return a map of task holder dimensions from the provided {@link
TaskHolder} if {@link TaskHolder#getDataSource()}
+ * and {@link TaskHolder#getTaskId()} are non-null.
+ * <p>The task ID ({@link TaskHolder#getTaskId()}) is added to both {@link
DruidMetrics#TASK_ID}
+ * and {@link DruidMetrics#ID} dimensions to the map for backward
compatibility. {@link DruidMetrics#ID} is
+ * deprecated because it's ambiguous and will be removed in a future
release.</p>
*/
- public static Map<String, String[]> mapOfDatasourceAndTaskID(
- @Nullable final String datasource,
- @Nullable final String taskId
- )
+ public static Map<String, String[]> mapOfTaskHolderDimensions(final
TaskHolder taskHolder)
Review Comment:
I think it would now make sense to move this method into `TaskHolder`
itself. The map would be created just once when the `TaskHolder` instance is
created.
Then we can just call `taskHolder.getMetricDimensions()` and get the same
immutable wherever needed.
##########
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java:
##########
@@ -150,50 +149,46 @@ public MonitorScheduler getMonitorScheduler(
@Provides
@ManageLifecycle
public JvmMonitor getJvmMonitor(
- DataSourceTaskIdHolder dataSourceTaskIdHolder
+ TaskHolder taskHolder
)
{
- Map<String, String[]> dimensions = MonitorsConfig.mapOfDatasourceAndTaskID(
- dataSourceTaskIdHolder.getDataSource(),
- dataSourceTaskIdHolder.getTaskId()
+ Map<String, String[]> dimensions =
MonitorsConfig.mapOfTaskHolderDimensions(
+ taskHolder
);
return new JvmMonitor(dimensions);
}
@Provides
@ManageLifecycle
public JvmCpuMonitor getJvmCpuMonitor(
- DataSourceTaskIdHolder dataSourceTaskIdHolder
+ TaskHolder taskHolder
)
{
- Map<String, String[]> dimensions = MonitorsConfig.mapOfDatasourceAndTaskID(
- dataSourceTaskIdHolder.getDataSource(),
- dataSourceTaskIdHolder.getTaskId()
+ Map<String, String[]> dimensions =
MonitorsConfig.mapOfTaskHolderDimensions(
+ taskHolder
);
return new JvmCpuMonitor(dimensions);
}
@Provides
@ManageLifecycle
- public JvmThreadsMonitor getJvmThreadsMonitor(DataSourceTaskIdHolder
dataSourceTaskIdHolder)
+ public JvmThreadsMonitor getJvmThreadsMonitor(TaskHolder taskHolder)
{
- Map<String, String[]> dimensions = MonitorsConfig.mapOfDatasourceAndTaskID(
- dataSourceTaskIdHolder.getDataSource(),
- dataSourceTaskIdHolder.getTaskId()
+ Map<String, String[]> dimensions =
MonitorsConfig.mapOfTaskHolderDimensions(
+ taskHolder
Review Comment:
Nit: Can we put this (and other similar invocations of this new method) in a
single line?
##########
services/src/main/java/org/apache/druid/cli/PeonLoadSpecHolder.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.cli;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.server.coordination.BroadcastDatasourceLoadingSpec;
+import org.apache.druid.server.lookup.cache.LookupLoadingSpec;
+import org.apache.druid.server.metrics.LoadSpecHolder;
+
+/**
+ * LoadSpecHolder implementation for peon processes.
+ *
+ * <p>This holder retrieves load specifications lazily via a {@link Provider}
to avoid
+ * circular dependencies during Guice initialization.</p>
+ */
+public class PeonLoadSpecHolder implements LoadSpecHolder
+{
+ private Provider<Task> taskProvider;
+
+ @Inject
+ public PeonLoadSpecHolder(
Review Comment:
Similar to `PeonTaskHolder`, this constructor should not need `@Inject`. Use
a `@Provides` method in `CliPeon` instead.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -380,7 +379,7 @@ private void takeSnapshot(Map<String,
LookupExtractorFactoryContainer> lookupMap
}
/**
- * Load a set of lookups based on the injected value in {@link
DataSourceTaskIdHolder#getLookupLoadingSpec()}.
+ * Load a set of lookups based on the injected value in {@link
TaskPropertiesHolder#getLookupLoadingSpec()}.
Review Comment:
```suggestion
* Load a set of lookups based on the injected value in {@link
LoadSpecHolder#getLookupLoadingSpec()}.
```
##########
services/src/main/java/org/apache/druid/cli/PeonTaskHolder.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.cli;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.server.metrics.TaskHolder;
+
+/**
+ * TaskHolder implementation for {@code CliPeon} processes.
+ *
+ * <p>This holder retrieves task information lazily via a {@link Provider} to
avoid
+ * circular dependencies during Guice initialization.</p>
+ */
+public class PeonTaskHolder implements TaskHolder
+{
+ private Provider<Task> taskProvider;
+
+ @Inject
+ public PeonTaskHolder(
Review Comment:
Suggestion: Do not use `@Inject` with this constructor and just pass a
`Task` into it instead of a `Provider`.
Use an `@Provides` method in `CliPeon` to bind it correctly.
```java
@Provides
public TaskHolder getTaskHolder(Task task)
{
return new PeonTaskHolder(task);
}
```
##########
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java:
##########
@@ -110,9 +108,10 @@ public MonitorScheduler getMonitorScheduler(
{
List<Monitor> monitors = new ArrayList<>();
// HACK: when ServiceStatusMonitor is the first to be loaded, it
introduces a circular dependency between
- // CliPeon.runTask and
CliPeon.getDataSourceFromTask/CliPeon.getTaskIDFromTask. The reason for this is
unclear
- // but by injecting DataSourceTaskIdHolder early this cycle is avoided.
- injector.getInstance(DataSourceTaskIdHolder.class);
+ // CliPeon.runTask and TaskHolder.getDataSource()/TaskHolder.getTaskId().
The reason for this is unclear
+ // but by injecting TaskPropertiesHolder early this cycle is avoided.
Review Comment:
Is this true even after the changes in this PR?
##########
server/src/test/java/org/apache/druid/server/metrics/GroupByStatsMonitorTest.java:
##########
@@ -106,24 +98,22 @@ public void testMonitorWithServiceDimensions()
{
final String dataSource = "fooDs";
final String taskId = "taskId1";
- final Injector injector = Initialization.makeInjectorWithModules(
- GuiceInjectors.makeStartupInjector(),
- List.of(binder -> {
- JsonConfigProvider.bindInstance(
- binder,
- Key.get(DruidNode.class, Self.class),
- new DruidNode("test-inject", null, false, null, null, true,
false)
- );
- binder.bind(Key.get(String.class,
Names.named(DataSourceTaskIdHolder.DATA_SOURCE_BINDING)))
- .toInstance(dataSource);
- binder.bind(Key.get(String.class,
Names.named(DataSourceTaskIdHolder.TASK_ID_BINDING)))
- .toInstance(taskId);
- })
- );
- final DataSourceTaskIdHolder dsTaskIdHolder = new DataSourceTaskIdHolder();
- injector.injectMembers(dsTaskIdHolder);
- final GroupByStatsMonitor monitor =
- new GroupByStatsMonitor(groupByStatsProvider, mergeBufferPool,
dsTaskIdHolder);
+ final GroupByStatsMonitor monitor = new GroupByStatsMonitor(
+ groupByStatsProvider,
+ mergeBufferPool,
+ new TaskHolder() {
Review Comment:
Maybe create a `TestTaskHolder` so that we don't have to create this inline
class in multiple places.
--
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]
