feat(cache-cell-results): added cell result caching to reduce cell configuration querying on loads #18581

asalem1 · 2020-06-17T20:28:44Z

Problem

Dashboard cells are queried once when they are initially loaded, then again when the cell is opened for configuration

Solution

Implemented a reducer-based cache to cache dashboard cell queries and their results based on a hashed query index. Now, if a cell is opened for configuration, the action will check to see if a result exists for that hashed query, otherwise the query will execute. Once the dashboard unmounts, the query is cached.

CHANGELOG.md updated with a link to the PR (not the Issue)
Rebased/mergeable

hoorayimhelping · 2020-06-18T18:02:45Z

ui/src/data/actions/index.ts

+  | ReturnType<typeof resetCachedQueryResults>
+  | ReturnType<typeof setQueryResultsByQueryID>
+
+export const hashCode = s =>


it's usually a good idea to paste the link you copy/pasted internet code form so people can get some context on it, and in the case of bugs, go right to the source.

hoorayimhelping · 2020-06-18T18:13:28Z

ui/src/data/reducers/index.ts

+  }
+
+  return state
+}


is this file necessary? why can't the query results be stored in the time machine reducer?

I had originally set the data in the time machine, but it became a little tricky accessing the data since timeMachine is partitioned into segmented subsets. The thinking with this feature is that we might be also benefit from this caching across the platform in the future, so if the same query is run in data-explorer and a dashboard cell, we'd be able to cache those results and reduce querying

hoorayimhelping · 2020-06-18T18:14:58Z

ui/src/shared/components/TimeSeries.tsx

@@ -344,6 +349,7 @@ const mstp = (state: AppState, props: OwnProps): StateProps => {

 const mdtp: DispatchProps = {
  notify: notifyAction,
+  setQueryResults: setQueryResultsByQueryID,


what's the idea behind aliasing this? it's really confusing - there's already a method being exported from another file called setQueryResults. this really confounds the ability to see where setQueryResults is used - this represents a false positive of that.

Fair point. I think aliasing helps differentiate between the imported action vs the one that's accessible in the props. Without aliasing, I find that my workflow is:

Import the action

Try and call the action directly within the component

Get confused for a bit, do some debugging, then realize the err of my ways

Having said that, I'm fine with not aliasing if it makes things clearer

for differentiate between the imported action vs the one that's accessible in the props we sometimes use setX vs onSetX I had personally set a bad example of this in this file. :(

hoorayimhelping · 2020-06-18T18:21:50Z

ui/src/views/actions/thunks.ts

+    dispatch(executeQueries())
+  } catch (error) {
+    // if the files don't exist in the cache, we want to execute the query
+    console.error(error)


if i'm understanding this correctly, this logs an error on cache misses? seems noisy - if that's the case i think we should just swallow this error and not log it.

Good point.

hoorayimhelping · 2020-06-18T18:23:45Z

ui/src/dashboards/components/DashboardPage.tsx


 @ErrorHandling
 class DashboardPage extends Component<Props> {
+  public componentWillUnmount() {
+    this.props.resetCachedQueryResults()


👍 thinking about cache invalidation

Can't take any credit for it, it was all @ebb-tide 's idea

hoorayimhelping · 2020-06-18T18:25:29Z

ui/src/types/stores.ts

@@ -40,6 +43,7 @@ export interface AppState {
  }
  currentPage: CurrentPage
  currentDashboard: CurrentDashboardState
+  data: DataState


i'm going to be blunt here: data is a really poor name for a reducer - all reducers deal in data. i'm not sure if this reducer needs to exist, but if it does, what's wrong with calling it the cache reducer or the queryCache reducer?

Those are really great names, I'll switch them up

ebb-tide · 2020-06-18T17:22:40Z

ui/src/dashboards/components/DashboardPage.tsx


 @ErrorHandling
 class DashboardPage extends Component<Props> {
+  public componentWillUnmount() {
+    this.props.resetCachedQueryResults()


ebb-tide · 2020-06-18T18:08:27Z

ui/src/data/reducers/index.ts

+    }
+
+    case 'RESET_CACHED_QUERY_RESULTS': {
+      return initialState


let's return a brand new object here just in case.

ebb-tide · 2020-06-18T18:09:34Z

ui/src/store/configureStore.ts

@@ -73,6 +74,7 @@ export const rootReducer = combineReducers<ReducerState>({
  }),
  currentPage: currentPageReducer,
  currentDashboard: currentDashboardReducer,
+  data: dataReducer,


maybe call this queryCache and queryCacheReducer?. data can mean sooo many things... :/

totally. Just seeing this. how about dashboardQueryCache? or are we thinking something a little less specific?

I like queryCache, I can't tell if we will only use this in dashboards or not.

ebb-tide · 2020-06-18T18:13:42Z

ui/src/views/actions/thunks.ts

+    const state = getState()
+    const {view} = getActiveTimeMachine(state)
+    const queries = view.properties.queries.filter(({text}) => !!text.trim())
+    const queryID = get(queries, '[0].text', '')


The function name setQueryResultsByQueryID suggests that you will be hashing first and then passing queryID rather than queryText to the function. I think you should be hashing on this line.

Definitely. I was thinking that as I was walking through this time around. I'll go ahead and import it in and hash it here

ebb-tide · 2020-06-18T18:41:56Z

ui/src/views/actions/thunks.ts

+    const {view} = getActiveTimeMachine(state)
+    const queries = view.properties.queries.filter(({text}) => !!text.trim())
+    const queryID = get(queries, '[0].text', '')
+    if (queryID) {


This should be where you lookup to see whether the queryID exists in your cache.

in TimeSeries- we setQueryResults, here we should read it back from queryResults.
so if state.queryResults.Query.queryResultsByQueryID[queryID] exists we should use it in the visualization, and not executeQueries. (you might need to look into how to use in the visualization)

I think there might be a misunderstanding stemming from naming two different functions the same thing, since this dispatch should be dispatching the action above:

https://github.com/influxdata/influxdb/pull/18581/files#diff-d32f5a03b5c68c499b895f2222a64da9R125

and not the action to set the data

yes, that is pretty confusing :) I don't think we need a separate setQueryResultsByQueryID function here, just roll what it does into this action.

ebb-tide · 2020-06-18T18:49:16Z

ui/src/data/actions/index.ts

+export const hashCode = s =>
+  s.split('').reduce((a, b) => ((a << 5) - a + b.charCodeAt(0)) | 0, 0)
+
+export const setQueryResultsByQueryID = (queryID: string, files: string[]) =>


it's really confusing when the variable is called queryID but you are actually passing in the queryText, and you will convert it into the ID inside the function. Could you change the name of this function to setQueryResultsForQuery(queryText: string, files: string[]))

Really great point. I ended up importing the hashCode function into the TimeSeries component to set the ID there, so now it's actually passing the ID in

ebb-tide

sorry I'm not done!

ebb-tide · 2020-06-18T20:07:35Z

ui/src/data/reducers/index.ts

+    case 'SET_QUERY_RESULTS_BY_QUERY': {
+      return produce(state, draftState => {
+        const {queryID, files} = action
+        if (queryID && files.length) {


I think we should be checking these conditions before firing SET_QUERY_RESULTS_BY_QUERY action, and then we can rely on them being here.

Sounds good.

asalem1 · 2020-06-18T20:44:30Z

ui/src/timeMachine/actions/queries.ts

+    const {
+      alertBuilder: {id: checkID},
+    } = state
+


Moved this down so it would be place a little closer to the source of where checkID is being used

ebb-tide · 2020-06-18T20:53:00Z

ui/src/views/actions/thunks.ts

+): Promise<void> => {
+  try {
+    const state = getState()
+    const files = state.queryCache.queryResultsByQueryID[hashCode(queryID)]


You are hashing queryID before sending to this function. so you shouldn't be hashing here.

Hmmm, I'm not sure if i am, but I definitely should be since it's a little misleading at this point

ebb-tide · 2020-06-18T20:55:36Z

ui/src/shared/components/TimeSeries.tsx

@@ -271,6 +280,12 @@ class TimeSeries extends Component<Props & WithRouterProps, State> {

      this.pendingReload = false

+      const queryText = queries[0].text


This will not work if there is more than one query in queries

I was wondering about that. I've never seen a situation where more than one query is being sent, but is that a possibility? If so, should we cache each one separately?

ebb-tide · 2020-06-18T21:28:58Z

ui/src/views/actions/thunks.ts

+  timeMachineID: TimeMachineID
+) => (dispatch, getState: GetState): Promise<void> => {
+  try {
+    dispatch(getViewForTimeMachine(dashboardID, cellID, timeMachineID))


I'm worried about race conditions here. We can not await the results of a thunk, we just dispatch it, and so if we are depending on its results as we are... on line 151, we're hoping that getViewForTimeMachine has done its thing already and we can get the view. The solutions is to integrate the getViewForTimeMachine and setQueryResultsForCell functions in to one.

I hear what you're saying, it's definitely a valid concern, but it seems like it might be overstepping the intent of the function. I'm curious as to how moving the dispatches within the scope of setQueryResultsForCell would prevent a race condition?

ebb-tide · 2020-06-22T18:41:37Z

ui/src/views/actions/thunks.ts

+    dispatch(executeQueries())
+  } catch (error) {
+    // if the files don't exist in the cache, we want to execute the query
+    dispatch(executeQueries())


I don't think there is an error here that is relevant. We are already checking if files don't exist on line 132, and executing queries if they don't. on line 136

that's a really good point that I didn't consider. Should we just remove this from the try/catch?

I think so, yeah.

ui/src/views/actions/thunks.ts

ebb-tide · 2020-06-22T19:24:01Z

ui/src/dashboards/components/EditVEO.tsx

  view,
 }) => {
  useEffect(() => {
    // TODO split this up into "loadView" "setActiveTimeMachine"
    // and something to tell the component to pull from the context
    // of the dashboardID
-    getViewForTimeMachine(dashboardID, cellID, 'veo')
+    setQueryResultsForCell(dashboardID, cellID, 'veo')


ok this is a nit, but this function name is a lot less descriptive to me. can we switch it to something that has a reference to view? maybe getViewAndResultsForVEO?

spoke offline and addressed the issues

…ved as a means of caching

… separated some of the view thunk logic to. Need to add more explicit typing and need to integrate cache expiration logic

…nt unmounts. Also separate thunk concerns to default query execution when files are undefined

Adding context to where the hashing function was found Renaming the data src to queryCache Renaming the imported function in TimeSeries Renaming some of the input parameters to be a little more relevant

…ased on the query text and renamed var to align with realistic output

…t in the output file

…ved as a means of caching

… separated some of the view thunk logic to. Need to add more explicit typing and need to integrate cache expiration logic

…nt unmounts. Also separate thunk concerns to default query execution when files are undefined

Adding context to where the hashing function was found Renaming the data src to queryCache Renaming the imported function in TimeSeries Renaming some of the input parameters to be a little more relevant

Co-authored-by: Deniz Kusefoglu <[email protected]>

…ctionality of getViewAndResultsForVeo to eliminate possible race condition

asalem1 requested review from hoorayimhelping and ebb-tide June 17, 2020 20:28

asalem1 changed the title ~~Feat/conditionally query cell~~ feat(cache-cell-results): added cell result caching to reduce cell configuration querying on loads Jun 17, 2020

asalem1 force-pushed the feat/conditionally-query-cell branch from cb0a5ae to 27e4c32 Compare June 17, 2020 22:05

hoorayimhelping previously requested changes Jun 18, 2020

View reviewed changes

ebb-tide approved these changes Jun 18, 2020

View reviewed changes

ebb-tide suggested changes Jun 18, 2020

View reviewed changes

ebb-tide reviewed Jun 18, 2020

View reviewed changes

asalem1 force-pushed the feat/conditionally-query-cell branch from 5a61d99 to 57c49a0 Compare June 18, 2020 20:37

asalem1 requested review from ebb-tide and hoorayimhelping June 18, 2020 20:37

asalem1 commented Jun 18, 2020

View reviewed changes

ebb-tide reviewed Jun 18, 2020

View reviewed changes

asalem1 requested a review from ebb-tide June 18, 2020 20:57

ebb-tide reviewed Jun 18, 2020

View reviewed changes

asalem1 force-pushed the feat/conditionally-query-cell branch from 593db7b to cec9b0d Compare June 18, 2020 22:23

asalem1 requested a review from ebb-tide June 22, 2020 12:29

ebb-tide reviewed Jun 22, 2020

View reviewed changes

ui/src/views/actions/thunks.ts Outdated Show resolved Hide resolved

ebb-tide reviewed Jun 22, 2020

View reviewed changes

ebb-tide approved these changes Jun 22, 2020

View reviewed changes

asalem1 force-pushed the feat/conditionally-query-cell branch from 638733e to ab3dfd3 Compare June 22, 2020 22:24

asalem1 force-pushed the feat/conditionally-query-cell branch from 1b09f53 to 2179681 Compare June 23, 2020 12:32

asalem1 added 5 commits June 23, 2020 08:33

feat(data-reducer): setting file data once a dashboard query is resol…

eaa62a4

…ved as a means of caching

feat(view_action): reorganized the actions, combined the reducers and…

2d4c896

… separated some of the view thunk logic to. Need to add more explicit typing and need to integrate cache expiration logic

feat(conditionally-query-cell): Added cache clearing when the compone…

0c1116e

…nt unmounts. Also separate thunk concerns to default query execution when files are undefined

chore(CHANGELOG): updated changelog

0d40fbd

chore(lint): removed async to resolve lint issues

c93801e

asalem1 and others added 13 commits June 23, 2020 08:33

chore(PR-suggestions): made suggested PR changes by

b6c0887

Adding context to where the hashing function was found Renaming the data src to queryCache Renaming the imported function in TimeSeries Renaming some of the input parameters to be a little more relevant

chore(comment): removed unnecessary comment

120dd94

fix(hashcode-placement): fixed where the queryID is being generated b…

3965a0c

…ased on the query text and renamed var to align with realistic output

fix(multiple_queries): handling the case where multiple queries resul…

4370a04

…t in the output file

feat(data-reducer): setting file data once a dashboard query is resol…

9a4c885

…ved as a means of caching

feat(view_action): reorganized the actions, combined the reducers and…

c1e7bce

… separated some of the view thunk logic to. Need to add more explicit typing and need to integrate cache expiration logic

feat(conditionally-query-cell): Added cache clearing when the compone…

69bfb40

…nt unmounts. Also separate thunk concerns to default query execution when files are undefined

chore(CHANGELOG): updated changelog

5d6d1cd

chore(PR-suggestions): made suggested PR changes by

fc2db5b

Adding context to where the hashing function was found Renaming the data src to queryCache Renaming the imported function in TimeSeries Renaming some of the input parameters to be a little more relevant

fix(pr): merging suggested PR change

6e5a19e

Co-authored-by: Deniz Kusefoglu <[email protected]>

chore(rename-func): renamed func for greater accuracy and updated fun…

014ec9e

…ctionality of getViewAndResultsForVeo to eliminate possible race condition

fix(branch-weirdness): fixed weirdness on the branch

a7e46d8

chore(lint): fixed linter error

655d5de

asalem1 force-pushed the feat/conditionally-query-cell branch from 2179681 to 655d5de Compare June 23, 2020 15:33

asalem1 merged commit c4fee5b into master Jun 23, 2020

asalem1 deleted the feat/conditionally-query-cell branch June 23, 2020 16:34

		@@ -271,6 +280,12 @@ class TimeSeries extends Component<Props & WithRouterProps, State> {

		this.pendingReload = false

		const queryText = queries[0].text

feat(cache-cell-results): added cell result caching to reduce cell configuration querying on loads #18581

feat(cache-cell-results): added cell result caching to reduce cell configuration querying on loads #18581

Conversation

asalem1 commented Jun 17, 2020 • edited Loading

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

ebb-tide Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 commented Jun 17, 2020 •

edited

Loading

asalem1 Jun 18, 2020 •

edited

Loading

ebb-tide Jun 18, 2020 •

edited

Loading

ebb-tide Jun 18, 2020 •

edited

Loading