You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/02/19 00:23:14 UTC
[GitHub] [druid] maytasm opened a new pull request #10900: Add query granularity to compaction task
maytasm opened a new pull request #10900:
URL: https://github.com/apache/druid/pull/10900
Add query granularity to compaction task
### Description
Add query granularity to compaction task. Note that query granularity is still not supported in auto compaction.
This PR also creates a new class `CompactionGranularitySpec` to use instead of `GranularitySpec` when passing query granularity and segment granularity into compaction task. This allows the null value (value not given by the user) to represent using the original current query granularity and segment granularity (this is the existing behavior for compaction task). Note that the Compaction task ultimately still converts `CompactionGranularitySpec` to a `UniformGranularitySpec` when creating the index ingestion spec
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r580698451
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
This class is a GranularitySpec that is only applicable for Auto-Compaction/Compaction task. It contains more than just a Granularity. Similar to UniformGranularitySpec, it contains segmentGranularity and queryGranularity. In my next PR which will adds rollup to Auto-Compaction/Compaction task, this class will also contains option to enable/disable rollup. It is different to UniformGranularitySpec that it doesn't support inputIntervals. There is enough distinction between GranularitySpec for Compaction task than normal Index task that I decided to create a new GranularitySpec class for Compaction task.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm merged pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #10900:
URL: https://github.com/apache/druid/pull/10900
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r581199468
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
In reality, even though they sound similar, the segment granularity and the query granularity are different, orthogonal concepts. I think it was a mistake to put them together since the GranularitySpec interface was introduced. I am not 100% sure that it is a mistake but it sure feels this way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r580676685
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
This name is confusing to me since this class represents a `Granularity` not a `GranularitySpec`. Maybe `CompactionGranularities` is a better name? Also the introduction of this shallow class seems like it is adding to the cognitive load of the reader...was there other cleaner way to support the "null" case out of which this class was created?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm edited a comment on pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm edited a comment on pull request #10900:
URL: https://github.com/apache/druid/pull/10900#issuecomment-789152703
> Assuming the doc changes are coming in a follow up change - we should make it clear what the tradeoffs are for rolling up data.
>
> If someone accidentally rolls up data to a coarser query granularity (MONTH -> YEAR) - do they have any way to get the segments with the finer queryGranularity back (MONTH)?
>
> > Note that the Compaction task ultimately still converts CompactionGranularitySpec to a UniformGranularitySpec when creating the index ingestion spec
>
> I don't understand the granularity specs enough. What's the impact of using UniformGranularitySpec instead of ArbitraryGranularitySpec? Is this something a user should be aware of?
The doc change is coming in a separate PR (https://github.com/apache/druid/pull/10935). I will make it clear that If someone rolls up data to a coarser query granularity (MONTH -> YEAR) -the segment with finer queryGranularity (MONTH) will be overshadowed. Those segments may be remove from deep storage if a kill task is run on those intervals. Hence, user can lose data with finer queryGranularity (MONTH).
Regarding UniformGranularitySpec vs. ArbitraryGranularitySpec. This was a design choice of existing implementation. IT was not changed in this PR. It is not something a user should be aware of. UniformGranularitySpec works much better in this case as Druid will automatically buckets down the interval according to the segmentGranularity. A ArbitraryGranularitySpec requires you to explicitly list out all bucket intervals.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r580702384
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
SegmentGranularity that is in CompactionTask class was already Deprecated and will be removed in https://github.com/apache/druid/pull/10912. This is in favor of combining SegmentGranularity and QueryGranularity and other Compaction task granularity-related configs (such as rollup which will be added in my next PR) in CompactionGranularitySpec class
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585716191
##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/ITCompactionTaskTest.java
##########
@@ -87,11 +92,68 @@ public void testCompactionWithSegmentGranularity() throws Exception
}
@Test
- public void testCompactionWithGranularitySpec() throws Exception
+ public void testCompactionWithSegmentGranularityInGranularitySpec() throws Exception
{
loadDataAndCompact(INDEX_TASK, INDEX_QUERIES_RESOURCE, COMPACTION_TASK_WITH_GRANULARITY_SPEC, GranularityType.MONTH);
}
+ @Test
+ public void testCompactionWithQueryGranularityInGranularitySpec() throws Exception
+ {
+ try (final Closeable ignored = unloader(fullDatasourceName)) {
+ loadData(INDEX_TASK);
+ // 4 segments across 2 days
+ checkNumberOfSegments(4);
+ List<String> expectedIntervalAfterCompaction = coordinator.getSegmentIntervals(fullDatasourceName);
+ expectedIntervalAfterCompaction.sort(null);
+
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.SECOND.name(), 4);
+ String queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ // QueryGranularity was SECOND, now we will change it to HOUR
+ compactData(COMPACTION_TASK_WITH_GRANULARITY_SPEC, null, GranularityType.HOUR);
+
+ // The original 4 segments should be compacted into 2 new segments
+ checkNumberOfSegments(2);
+ queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_HOUR_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.HOUR.name(), 2);
+ checkCompactionIntervals(expectedIntervalAfterCompaction);
+ }
+ }
+
+ @Test
+ public void testCompactionWithSegmentGranularityAndQueryGranularityInGranularitySpec() throws Exception
+ {
+ try (final Closeable ignored = unloader(fullDatasourceName)) {
+ loadData(INDEX_TASK);
+ // 4 segments across 2 days
+ checkNumberOfSegments(4);
+ List<String> expectedIntervalAfterCompaction = coordinator.getSegmentIntervals(fullDatasourceName);
+ expectedIntervalAfterCompaction.sort(null);
+
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.SECOND.name(), 4);
+ String queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ compactData(COMPACTION_TASK_WITH_GRANULARITY_SPEC, GranularityType.YEAR, GranularityType.YEAR);
+
+ // The original 4 segments should be compacted into 1 new segment
+ checkNumberOfSegments(1);
+ queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_YEAR_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.YEAR.name(), 1);
+
+ List<String> newIntervals = new ArrayList<>();
+ for (String interval : expectedIntervalAfterCompaction) {
+ for (Interval newinterval : GranularityType.YEAR.getDefaultGranularity().getIterable(new Interval(interval, ISOChronology.getInstanceUTC()))) {
+ newIntervals.add(newinterval.toString());
+ }
+ }
+ expectedIntervalAfterCompaction = newIntervals;
+ checkCompactionIntervals(expectedIntervalAfterCompaction);
+ }
+ }
+
Review comment:
Can you add a test for what happens when someone tries to go from a larger query granularity to a smaller query granularity - like going from data rolled up to the YEAR, and trying to unroll it to MONTH. I assume the compaction task would fail. What is the error message in this case. Is it clear to the user what the problem is?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #10900:
URL: https://github.com/apache/druid/pull/10900#issuecomment-789152703
> Assuming the doc changes are coming in a follow up change - we should make it clear what the tradeoffs are for rolling up data.
>
> If someone accidentally rolls up data to a coarser query granularity (MONTH -> YEAR) - do they have any way to get the segments with the finer queryGranularity back (MONTH)?
>
> > Note that the Compaction task ultimately still converts CompactionGranularitySpec to a UniformGranularitySpec when creating the index ingestion spec
>
> I don't understand the granularity specs enough. What's the impact of using UniformGranularitySpec instead of ArbitraryGranularitySpec? Is this something a user should be aware of?
The doc change is coming in a separate PR. I will make it clear that If someone rolls up data to a coarser query granularity (MONTH -> YEAR) -the segment with finer queryGranularity (MONTH) will be overshadowed. Those segments may be remove from deep storage if a kill task is run on those intervals. Hence, user can lose data with finer queryGranularity (MONTH).
Regarding UniformGranularitySpec vs. ArbitraryGranularitySpec. This was a design choice of existing implementation. IT was not changed in this PR. It is not something a user should be aware of. UniformGranularitySpec works much better in this case as Druid will automatically buckets down the interval according to the segmentGranularity. A ArbitraryGranularitySpec requires you to explicitly list out all bucket intervals.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r581200702
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
Segment granularity plays well with intervals and buckets. Query granularity plays well with rollup (I feel query granularity for ingestion/compaction ought have to be named rollupGranularity instead).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585699302
##########
File path: server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskGranularitySpec.java
##########
@@ -22,26 +22,22 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.java.util.common.granularity.Granularity;
-import org.apache.druid.segment.indexing.granularity.BaseGranularitySpec;
import java.util.Objects;
-public class ClientCompactionTaskQueryGranularitySpec
+public class ClientCompactionTaskGranularitySpec
Review comment:
Can you add a javadoc for this please. Why is this needed? Why we haven't chosen to use the GranularitySpec class that already exists?
I'm ok if you do this in a follow up change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585714231
##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/ITCompactionTaskTest.java
##########
@@ -164,18 +211,31 @@ private void loadData(String indexTask) throws Exception
);
}
- private void compactData(String compactionResource, GranularityType newSegmentGranularity) throws Exception
+ private void compactData(String compactionResource, GranularityType newSegmentGranularity, GranularityType newQueryGranularity) throws Exception
{
String template = getResourceAsString(compactionResource);
template = StringUtils.replace(template, "%%DATASOURCE%%", fullDatasourceName);
+ // For the new granularitySpec map
+ Map<String, String> granularityMap = new HashMap<>();
+ if (newSegmentGranularity != null) {
+ granularityMap.put("segmentGranularity", newSegmentGranularity.name());
+ }
+ if (newQueryGranularity != null) {
+ granularityMap.put("queryGranularity", newQueryGranularity.name());
+ }
+ template = StringUtils.replace(
+ template,
+ "%%GRANULARITY_SPEC%%",
+ jsonMapper.writeValueAsString(granularityMap)
+ );
Review comment:
I think this means that there will always be a granulartySpec provided as part of the compaction task spec. Do we have a test running a compact task without specifying a granularitySpec. I'm trying to think of what happens in an upgrade/downgrade scenario where the compaction job being submitted may not have the granularitySpec as part of the compaction spec
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585835212
##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/ITCompactionTaskTest.java
##########
@@ -164,18 +211,31 @@ private void loadData(String indexTask) throws Exception
);
}
- private void compactData(String compactionResource, GranularityType newSegmentGranularity) throws Exception
+ private void compactData(String compactionResource, GranularityType newSegmentGranularity, GranularityType newQueryGranularity) throws Exception
{
String template = getResourceAsString(compactionResource);
template = StringUtils.replace(template, "%%DATASOURCE%%", fullDatasourceName);
+ // For the new granularitySpec map
+ Map<String, String> granularityMap = new HashMap<>();
+ if (newSegmentGranularity != null) {
+ granularityMap.put("segmentGranularity", newSegmentGranularity.name());
+ }
+ if (newQueryGranularity != null) {
+ granularityMap.put("queryGranularity", newQueryGranularity.name());
+ }
+ template = StringUtils.replace(
+ template,
+ "%%GRANULARITY_SPEC%%",
+ jsonMapper.writeValueAsString(granularityMap)
+ );
Review comment:
There are compaction IT (existing ones) still use compaction spec (/indexer/wikipedia_compaction_task.json) without granularitySpec (for example, `testCompaction()`).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r580694252
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
Why not simply pass QueryGranularity (similar to SegmentGranularity already being added) to CompactionTask rather than creating a whole new class (CompactionGranularitySpec)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585704656
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/UserCompactionTaskGranularityConfig.java
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.server.coordinator;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class UserCompactionTaskGranularityConfig
Review comment:
javadocs please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585835894
##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/ITCompactionTaskTest.java
##########
@@ -164,18 +211,31 @@ private void loadData(String indexTask) throws Exception
);
}
- private void compactData(String compactionResource, GranularityType newSegmentGranularity) throws Exception
+ private void compactData(String compactionResource, GranularityType newSegmentGranularity, GranularityType newQueryGranularity) throws Exception
{
String template = getResourceAsString(compactionResource);
template = StringUtils.replace(template, "%%DATASOURCE%%", fullDatasourceName);
+ // For the new granularitySpec map
+ Map<String, String> granularityMap = new HashMap<>();
+ if (newSegmentGranularity != null) {
+ granularityMap.put("segmentGranularity", newSegmentGranularity.name());
+ }
+ if (newQueryGranularity != null) {
+ granularityMap.put("queryGranularity", newQueryGranularity.name());
+ }
+ template = StringUtils.replace(
+ template,
+ "%%GRANULARITY_SPEC%%",
+ jsonMapper.writeValueAsString(granularityMap)
+ );
Review comment:
Basically,
```
template = StringUtils.replace(
template,
"%%GRANULARITY_SPEC%%",
jsonMapper.writeValueAsString(granularityMap)
);
```
will be a no-op for those tests that uses compaction spec without granularitySpec (/indexer/wikipedia_compaction_task.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585833837
##########
File path: server/src/test/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigTest.java
##########
@@ -238,7 +234,7 @@ public void testSerdeGranularitySpec() throws IOException
null,
new Period(3600),
null,
- new UniformGranularitySpec(Granularities.HOUR, null, null),
Review comment:
No it does not. The json serialization / deserialization key remains the same
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r583927579
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
Removed this class as it is not needed. We already have ClientCompactionTaskGranularitySpec class which is use for passing around the Compaction task's ingestionSpec's granularities (segmentGranularity, queryGranularity).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r581199468
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
In reality, even though they sound similar, the segment granularity and the query granularity are different, orthogonal concepts. I think it was a mistake to put them together since the GranularitySpec interface was introduced. I am not 100% sure that it is a mistake but it sure feels this way.
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
Segment granularity plays well with intervals and buckets. Query granularity plays well with rollup (I feel query granularity for ingestion/compaction ought have to be named rollupGranularity instead).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r581286101
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
There is an opportunity here to unify the "granularity spec" design. The same design (i.e. class model) could be used for ingestion & compaction, unified. That may require refactoring of the "GranularitySpec" interface/class hierarchy though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585708703
##########
File path: server/src/test/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigTest.java
##########
@@ -238,7 +234,7 @@ public void testSerdeGranularitySpec() throws IOException
null,
new Period(3600),
null,
- new UniformGranularitySpec(Granularities.HOUR, null, null),
Review comment:
Does this change have any impact for upgrades / downgrades?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r580699406
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
This class will not only contains `Granularity` hence, it is named `CompactionGranularitySpec`. I do not think it adds much cognitive load of the reader since it is a very simple POJO. I think it would be much more cognitive load to try to fit the existing UniformGranularitySpec here since it does not fit well with what Compaction task supports / doesn't supports
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r580676685
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
This name is confusing to me since this class contains `Granularity`s not `GranularitySpec`s. Maybe `CompactionGranularities` is a better name? Also the introduction of this shallow class seems like it is adding to the cognitive load of the reader...was there other cleaner way to support the "null" case out of which this class was created?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r581199468
##########
File path: server/src/main/java/org/apache/druid/segment/indexing/granularity/CompactionGranularitySpec.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.segment.indexing.granularity;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class CompactionGranularitySpec
Review comment:
In reality there even though they sound similar, the segment granularity and the query granularity are different, orthogonal concepts. I think it was a mistake to put them together since the GranularitySpec interface was introduced. I am not 100% sure that it is a mistake but it sure feels this way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585832996
##########
File path: server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskGranularitySpec.java
##########
@@ -22,26 +22,22 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.java.util.common.granularity.Granularity;
-import org.apache.druid.segment.indexing.granularity.BaseGranularitySpec;
import java.util.Objects;
-public class ClientCompactionTaskQueryGranularitySpec
+public class ClientCompactionTaskGranularitySpec
Review comment:
I'll add in a followup change
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/UserCompactionTaskGranularityConfig.java
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.server.coordinator;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.java.util.common.granularity.Granularity;
+
+import java.util.Objects;
+
+public class UserCompactionTaskGranularityConfig
Review comment:
I'll add in a followup change
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #10900:
URL: https://github.com/apache/druid/pull/10900#issuecomment-788557024
LGTM after CI
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585699302
##########
File path: server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskGranularitySpec.java
##########
@@ -22,26 +22,22 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.java.util.common.granularity.Granularity;
-import org.apache.druid.segment.indexing.granularity.BaseGranularitySpec;
import java.util.Objects;
-public class ClientCompactionTaskQueryGranularitySpec
+public class ClientCompactionTaskGranularitySpec
Review comment:
Can you add a javadoc for this please. Why is this needed? Why we haven't chosen to use the GranularitySpec class that already exists? Whats the relationship between this and `UserCompactionTaskGranularityConfig`
I'm ok if you do this in a follow up change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10900:
URL: https://github.com/apache/druid/pull/10900#discussion_r585837666
##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/ITCompactionTaskTest.java
##########
@@ -87,11 +92,68 @@ public void testCompactionWithSegmentGranularity() throws Exception
}
@Test
- public void testCompactionWithGranularitySpec() throws Exception
+ public void testCompactionWithSegmentGranularityInGranularitySpec() throws Exception
{
loadDataAndCompact(INDEX_TASK, INDEX_QUERIES_RESOURCE, COMPACTION_TASK_WITH_GRANULARITY_SPEC, GranularityType.MONTH);
}
+ @Test
+ public void testCompactionWithQueryGranularityInGranularitySpec() throws Exception
+ {
+ try (final Closeable ignored = unloader(fullDatasourceName)) {
+ loadData(INDEX_TASK);
+ // 4 segments across 2 days
+ checkNumberOfSegments(4);
+ List<String> expectedIntervalAfterCompaction = coordinator.getSegmentIntervals(fullDatasourceName);
+ expectedIntervalAfterCompaction.sort(null);
+
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.SECOND.name(), 4);
+ String queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ // QueryGranularity was SECOND, now we will change it to HOUR
+ compactData(COMPACTION_TASK_WITH_GRANULARITY_SPEC, null, GranularityType.HOUR);
+
+ // The original 4 segments should be compacted into 2 new segments
+ checkNumberOfSegments(2);
+ queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_HOUR_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.HOUR.name(), 2);
+ checkCompactionIntervals(expectedIntervalAfterCompaction);
+ }
+ }
+
+ @Test
+ public void testCompactionWithSegmentGranularityAndQueryGranularityInGranularitySpec() throws Exception
+ {
+ try (final Closeable ignored = unloader(fullDatasourceName)) {
+ loadData(INDEX_TASK);
+ // 4 segments across 2 days
+ checkNumberOfSegments(4);
+ List<String> expectedIntervalAfterCompaction = coordinator.getSegmentIntervals(fullDatasourceName);
+ expectedIntervalAfterCompaction.sort(null);
+
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.SECOND.name(), 4);
+ String queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ compactData(COMPACTION_TASK_WITH_GRANULARITY_SPEC, GranularityType.YEAR, GranularityType.YEAR);
+
+ // The original 4 segments should be compacted into 1 new segment
+ checkNumberOfSegments(1);
+ queryResponseTemplate = getQueryResponseTemplate(INDEX_QUERIES_YEAR_RESOURCE);
+ queryHelper.testQueriesFromString(queryResponseTemplate);
+ checkQueryGranularity(SEGMENT_METADATA_QUERY_RESOURCE, GranularityType.YEAR.name(), 1);
+
+ List<String> newIntervals = new ArrayList<>();
+ for (String interval : expectedIntervalAfterCompaction) {
+ for (Interval newinterval : GranularityType.YEAR.getDefaultGranularity().getIterable(new Interval(interval, ISOChronology.getInstanceUTC()))) {
+ newIntervals.add(newinterval.toString());
+ }
+ }
+ expectedIntervalAfterCompaction = newIntervals;
+ checkCompactionIntervals(expectedIntervalAfterCompaction);
+ }
+ }
+
Review comment:
That functionality is not changed in this PR. It will be the same as doing a reindex task with larger query granularity to a smaller query granularity. Compaction task will reports the result of the index task (which would be the same as doing a reindex task)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10900: Add query granularity to compaction task
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10900:
URL: https://github.com/apache/druid/pull/10900#issuecomment-789040463
Assuming the doc changes are coming in a follow up change - we should make it clear what the tradeoffs are for rolling up data.
If someone accidentally rolls up data to a coarser query granularity (MONTH -> YEAR) - do they have any way to get the segments with the finer queryGranularity back (MONTH)?
> Note that the Compaction task ultimately still converts CompactionGranularitySpec to a UniformGranularitySpec when creating the index ingestion spec
I don't understand the granularity specs enough. What's the impact of using UniformGranularitySpec instead of ArbitraryGranularitySpec? Is this something a user should be aware of?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org