refactor CompactionState since 'core' and 'processing' modules have been merged#17741
Conversation
…een merged changes: * CompactionState now uses real types instead of Map * Rename ClientCompactionTaskTransformSpec to CompactionTransformSpec and move to processing, deleted UserCompactionTaskTransformConfig since it was the same as CompactionTransformSpec * Moved GranularitySpec and implementations to processing module
github-actions
bot
added
Area - Batch Ingestion
Area - Querying
Area - Segment Format and Ser/De
Area - Streaming Ingestion
Kubernetes
Area - Ingestion
Area - MSQ
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Thanks a lot for the cleanup, @clintropolis !
Left some minor comments.
Only blocking suggestion is to revert the formatting done in ControllerImpl.java since that is an involved class and it would be best to keep the diff there to a minimum.
| IndexSpec.class | ||
| ), | ||
| ImmutableMap.of() | ||
| jsonMapper.convertValue(ImmutableMap.of(), GranularitySpec.class) |
There was a problem hiding this comment.
Might be nice to add a GranularitySpec.DEFAULT in a later PR.
There was a problem hiding this comment.
yea, I was just leaving these as is to preserve the way stuff was being tested exactly instead of what they were effectively doing, not sure how useful that is though
| totalProcessedBytes = | ||
| msqTaskReportPayload.getCounters() | ||
| .copyMap() | ||
| .entrySet() | ||
| .stream() | ||
| .filter(entry -> stagesReport == null || stagesToInclude.contains(entry.getKey())) | ||
| .flatMap(counterSnapshotsMap -> counterSnapshotsMap.getValue() | ||
| .values() | ||
| .stream()) | ||
| .flatMap(counterSnapshots -> counterSnapshots.getMap() | ||
| .entrySet() | ||
| .stream()) | ||
| .filter(entry -> entry.getKey().startsWith("input")) | ||
| .mapToLong(entry -> { | ||
| ChannelCounters.Snapshot snapshot = (ChannelCounters.Snapshot) entry.getValue(); | ||
| return snapshot.getBytes() == null | ||
| ? 0L | ||
| : Arrays.stream(snapshot.getBytes()).sum(); | ||
| }) | ||
| .sum(); |
There was a problem hiding this comment.
Is the reformat really needed here and in other places in this file?
Maybe we can limit the diff in this PR to only the modified code.
There was a problem hiding this comment.
so like I don't normally do the whole file, but when looking through this one I kept noticing things that were obviously incorrectly formatted and decided to just run the whole file through because it was so ugly. I can revert but I'm going to immediately turn around and open another PR to do the same thing, if you would prefer that I can go to the trouble...
There was a problem hiding this comment.
Sure, we can leave it as it is then.
There was a problem hiding this comment.
went ahead and reverted, will fix in follow-up
| ImmutableMap.of() | ||
| ImmutableList.of(new CountAggregatorFactory("count")), | ||
| new CompactionTransformSpec(new SelectorDimFilter("dim1", "foo", null)), | ||
| MAPPER.convertValue(ImmutableMap.of(), IndexSpec.class), |
There was a problem hiding this comment.
Maybe use IndexSpec.DEFAULT instead?
| Collections.singletonMap("test2", "map2") | ||
| ImmutableList.of(new CountAggregatorFactory("count")), | ||
| new CompactionTransformSpec(new SelectorDimFilter("dim1", "foo", null)), | ||
| MAPPER.convertValue(Collections.singletonMap("test", "map"), IndexSpec.class), |
There was a problem hiding this comment.
Since we are touching these lines anyway, Map.of and List.of could be used for less verbosity.
| MAPPER.convertValue(Collections.singletonMap("test", "map"), IndexSpec.class), | |
| MAPPER.convertValue(Map.of("test", "map"), IndexSpec.class), |
There was a problem hiding this comment.
oh yea, I guess we aren't stuck on java 8 anymore... i forgot these methods exist now.
server/src/test/java/org/apache/druid/server/compaction/NewestSegmentFirstPolicyTest.java
Outdated
Show resolved
Hide resolved
Follow-up to #17741 to fix some formatting in ControllerImpl, no functional changes
3 participants
Description
Trying #14932 again since the conflicts got a bit much.
changes: