[3/5] [Run Timeline] Add hourly data cache #21934
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
salazarm
changed the title
salazarm
changed the title
salazarm
changed the title
bengotow
previously approved these changes
May 20, 2024
js_modules/dagster-ui/packages/ui-core/src/runs/useRunsForTimeline.tsx
Outdated
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/runs/HourlyDataCache.tsx
Outdated
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/runs/__tests__/HourlyDataCache.test.tsx
Outdated
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/runs/fetchPaginatedBucketData.ts
Outdated
Show resolved
Hide resolved
|
Going to be adding more tests and changing strategy a bit here. Will update PR soon |
salazarm
dismissed
bengotow’s stale review
Going to add more tests + change strategy a little
alangenfeld
previously requested changes
May 20, 2024
salazarm
commented
May 21, 2024
| } | ||
| }); | ||
| Object.entries(buckets) | ||
| .sort((bucketA, bucketB) => COMMON_COLLATOR.compare(bucketA[0], bucketB[0])) |
There was a problem hiding this comment.
this stops the run timeline rows from shuffling around
salazarm
commented
May 21, 2024
| @@ -14,7 +14,7 @@ import {WorkspaceContext} from '../workspace/WorkspaceContext'; | |||
| import {buildRepoAddress} from '../workspace/buildRepoAddress'; | |||
| const LOOKAHEAD_HOURS = 1; | |||
| const ONE_HOUR = 60 * 60 * 1000; | |||
| const POLL_INTERVAL = 60 * 1000; | |||
| const POLL_INTERVAL = 5 * 1000; | |||
There was a problem hiding this comment.
Lowered the poll interval since we only fetch for the missing ranges.
Increased the poll interview for FutureTicks / Ongoing runs below.
|
Buildkite failures unrelated |
alangenfeld
approved these changes
May 21, 2024
There was a problem hiding this comment.
I think this looks solid, probably still worth @bengotow taking a peak since my typescript isn't the strongest
js_modules/dagster-ui/packages/ui-core/src/runs/HourlyDataCache/HourlyDataCache.tsx
Outdated
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/runs/fetchPaginatedBucketData.ts
Outdated
Show resolved
Hide resolved
nikomancy
pushed a commit
that referenced
this pull request
May 22, 2024
## Summary & Motivation Add an hourly data cache to the Run Timeline. We only cache completed run data and we only request data for time ranges that we don't already have data for. - how do we handle settling ongoing runs in to the completed bucket? - - when an ongoing run goes into completed it should have an updatedTimestamp that belongs to a range we havent fetched yet Updated the completed run bucketing query logic to fetch updatedAfter -> updatedBefore timestamps to avoid the overfetching that the current updatedAfter -> createdBefore buckets make.  updatedAfter -> updatedBefore buckets only capture range 3 and 4 boxes though:  To capture range 2 and range 1 we need runs that were createdBefore the right boundary and updated after the right boundary. Instead of querying for these runs specifically we rely on the fact that we have already queried for all the future buckets since we always paginate backwards. So we subscribe to the cache for that data and then filter for runs that started before the right boundary of the time range ## How I Tested These Changes Jest tests. Left the page open for half an hour and saw in progress runs transition to completed. Switched between 1hr / 6hr / 12hr / 24hr views and saw we only request time ranges that are missing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Add an hourly data cache to the Run Timeline.
We only cache completed run data and we only request data for time ranges that we don't already have data for.
Updated the completed run bucketing query logic to fetch updatedAfter -> updatedBefore timestamps to avoid the overfetching that the current updatedAfter -> createdBefore buckets make.
updatedAfter -> updatedBefore buckets only capture range 3 and 4 boxes though:
To capture range 2 and range 1 we need runs that were createdBefore the right boundary and updated
after the right boundary. Instead of querying for these runs specifically we rely on the fact that we have already queried for all the future buckets since we always paginate backwards.
So we subscribe to the cache for that data and then filter for runs that started before the right boundary of the time range
How I Tested These Changes
Jest tests.
Left the page open for half an hour and saw in progress runs transition to completed.
Switched between 1hr / 6hr / 12hr / 24hr views and saw we only request time ranges that are missing.