Add Supervisor UI component to view timeline of rules applied by a cascading reindexing supervisor#19051
Add Supervisor UI component to view timeline of rules applied by a cascading reindexing supervisor#19051capistrant wants to merge 14 commits intoapache:masterfrom
Conversation
...service/src/test/java/org/apache/druid/indexing/compact/CascadingReindexingTemplateTest.java
Fixed
Show fixed
Hide fixed
...service/src/test/java/org/apache/druid/indexing/compact/CascadingReindexingTemplateTest.java
Fixed
Show fixed
Hide fixed
...service/src/test/java/org/apache/druid/indexing/compact/CascadingReindexingTemplateTest.java
Fixed
Show fixed
Hide fixed
kfaraz
left a comment
There was a problem hiding this comment.
Pretty cool!
Left some minor suggestions.
Will try this out locally.
Would also be nice to get a review from @vogievetsky on the web-console stuff.
| private final AppliedSkipOffset applied; | ||
| private final NotAppliedSkipOffset notApplied; |
There was a problem hiding this comment.
Nit: These two classes feel a little unnecessary. We could have just have the actual fields in the class SkipOffsetInfo directly: type, period, a boolean for isApplied, a nullable effectiveEndTime mutually exclusive with a nullable errorMessage.
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Outdated
Show resolved
Hide resolved
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Outdated
Show resolved
Hide resolved
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Outdated
Show resolved
Hide resolved
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Outdated
Show resolved
Hide resolved
| "tuning-7d", | ||
| null, | ||
| Period.days(7), | ||
| new UserCompactionTaskQueryTuningConfig( |
There was a problem hiding this comment.
Didn't we add a builder for this class in the previous PR?
| /** | ||
| * Test validation error when granularity timeline is invalid | ||
| */ |
There was a problem hiding this comment.
Not really needed since the test name captures this already.
| /** | ||
| * Test that skipOffsetFromNow correctly skips intervals and populates skipOffset.applied | ||
| */ |
There was a problem hiding this comment.
Not really needed as the test name already captures this.
| // Older data (P90D) has DAY granularity, newer data (P30D) has HOUR granularity | ||
| // This means as we move from past to present, granularity gets finer (valid) | ||
| // But then if we add MONTH for recent data, it becomes coarser (invalid) | ||
| ReindexingRuleProvider provider = InlineReindexingRuleProvider.builder() |
There was a problem hiding this comment.
Style tip: Move the .builder() to the next line to make the whole thing feel less crowded.
| ReindexingRuleProvider provider = InlineReindexingRuleProvider.builder() | |
| ReindexingRuleProvider provider = InlineReindexingRuleProvider | |
| .builder() |
| * @param referenceTime the reference time to use for computing rule periods (typically DateTime.now()) | ||
| * @return a view of the reindexing timeline with intervals and their configs | ||
| */ | ||
| public ReindexingTimelineView getReindexingTimelineView(DateTime referenceTime) |
There was a problem hiding this comment.
I feel like this method should not live here as it kind of pollutes the template definition.
Either have the timeline be built using a static method like ReindexingTimelineView.fromTemplate(template, referenceTime) or have a separate builder class altogether.
There was a problem hiding this comment.
hmm. I wonder about how we'd manage to re-use a lot of the underlying private implementation details that are quite convoluted (unfortunately). Like creating the base timeline from segment gran rules and what not is all shared between the timeline creation and the task creation. I agree that it feels a little weird here, but I didn't know how else to approach it without either duping a lot of internal logic or exposing it to both somehow. will keep thinking though
|
@capistrant , I took the liberty to slightly update the title for clarity. Hope it seems less ambiguous now. |
|
Was trying out the embedded test in this PR to check out the timeline. @capistrant , in the snapshot below, what does the "4 rules applied" mean? Maybe we should include an info popup for each of the chips.
|
Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Fixed
Show fixed
Hide fixed
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Fixed
Show fixed
Hide fixed
...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
Fixed
Show fixed
Hide fixed
4 rules applied means that the config created for the interval is made up of 4 different reindexing rules. In this case based on the test I assume you are running, it is
You could confirm by clicking the view raw rules button. I think this is a UX issue. Hand up, I don't have any experience curating a good UX :). If I had to guess what UX decision is best here, it is to remove the As for deletion rules, N deletion rules can apply to any given interval. They accumulate (unlike other rule types). Every deletion rule that applies a search interval will be combined together.
Any interval where both of those apply would have 2 deletion rules. The chips are intended to give very quick reference to what is in the config for the given search interval. They are not strictly tied to rules even. In my pictures you see metrics and dims in separate chips, but both belong to the same rule. So maybe the deletion rules chip should actually be like 2 deletion filters or 2 deletion clauses. I didn't know how valuable chips would be for tuningConfig and ioConfig so those are not surfaced, likely leading to your initial confusion. You'd have to view the config payload or the raw rules to see them. |


Description
New Supervisor API (cascading reindexing supervisors only)
This generates a timeline of search intervals for reindexing with the effective sets of rules that are used to create the underlying inline compaction configs for the different intervals. This is the business end of the console UI described below
Console visualization of reindexing timeline (cascading reindexing supervisors only)
For cascading compaction supervisors you can get a timeline view of how rules from the rule provider(s) will apply to your datasource. You can view how skip offsets (if they are configured) will change the timeline too. This visualization is a great help for operators in understanding in how their underlying rules will be applied across the datasource history. I will add some screen shots for reference
Refactor
CompactionSupervisorTestEmbedded TestsSplit out the cascading template tests to their own class from the inline compaction supervisor tests. Sharing one file was getting convoluted. They share a base class for what makes sense.
Release note
Key changed/added classes in this PR
CascadingReindexingTemplateReindexingTimelineViewSupervisorResourceweb-console/src/components/reindexing-timeline/reindexing-timeline.scssThis PR has: