imply-cheddar commented on code in PR #17699:
URL: https://github.com/apache/druid/pull/17699#discussion_r1945205234
##########
server/src/test/java/org/apache/druid/server/TestClusterQuerySegmentWalker.java:
##########
@@ -74,15 +76,18 @@
*/
public class TestClusterQuerySegmentWalker implements QuerySegmentWalker
{
+ public static final String TIMELINES_KEY = "timelines";
+
private final Map<String, VersionedIntervalTimeline<String,
ReferenceCountingSegment>> timelines;
private final QueryRunnerFactoryConglomerate conglomerate;
@Nullable
private final QueryScheduler scheduler;
private final GroupByQueryConfig groupByQueryConfig;
private final EtagProvider etagProvider;
+ @Inject
TestClusterQuerySegmentWalker(
- Map<String, VersionedIntervalTimeline<String, ReferenceCountingSegment>>
timelines,
+ @Named(TIMELINES_KEY) Map<String, VersionedIntervalTimeline<String,
ReferenceCountingSegment>> timelines,
Review Comment:
The only "good" usage of a named annotation is when it is completely
encapsulated inside of a singular module (i.e. there's a binding to the named
item and then a provider method in the same module that depends on the naming).
This is because it's too easy for named things to proliferate and it becomes
hard to figure out how things are actually being setup.
In this case, it's unclear to me why you need the annotation at all.
Though, from looking at it, it looks like you are trying to push a map around
to multiple different things and have them all use the same map. In that case,
I'd suggest that you create a relatively simple `TestSegmentsBroker` class
which all of the places that would use this map can depend on and then you bind
that. This makes it more clear that you have a singular object that multiple
things are potentially updating the state of and reading the state from.
##########
sql/src/test/java/org/apache/druid/sql/calcite/util/FakeIndexTaskUtil.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.sql.calcite.util;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.jsontype.NamedType;
+import org.apache.druid.data.input.InputFormat;
+import org.apache.druid.data.input.InputSource;
+import org.apache.druid.data.input.impl.LocalInputSource;
+import org.apache.druid.quidem.ProjectPathUtils;
+import org.apache.druid.segment.indexing.DataSchema;
+import org.apache.druid.segment.indexing.IOConfig;
+import org.apache.druid.segment.indexing.IngestionSpec;
+import org.apache.druid.segment.indexing.TuningConfig;
+import org.apache.druid.sql.calcite.util.datasets.InputSourceBasedTestDataset;
+import org.apache.druid.sql.calcite.util.datasets.TestDataSet;
+
+import java.io.File;
+import java.io.IOException;
+
+/**
+ * Utility class to create {@link TestDataSet} from fake indexing tasks.
+ *
+ * Goal is to let the users utilize the ingestion api to create test data.
+ */
+public class FakeIndexTaskUtil
+{
+ public static TestDataSet makeDS(ObjectMapper objectMapper, File src)
+ {
+ try {
+ ObjectMapper om = objectMapper.copy();
+ om.registerSubtypes(new NamedType(MyIOConfigType.class,
"index_parallel"));
+ FakeIndexTask indexTask = om.readValue(src, FakeIndexTask.class);
+ FakeIngestionSpec spec = indexTask.spec;
Review Comment:
I probably would've used the object mapper to read it in as a `Map<String,
Object>`, then `get` on the keys I want to check and use
`objectMapper.convertValue()` to convert into the types that you want. What
you've done works as well, but it's a little more difficult to read this code
and figure out what structure it wants in the JSON.
--
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]
