swith direct index query to api call #312
Conversation
ylwu-amzn
requested review from
yizheliu-amazon,
ohltyler and
saratvemulapalli
| [SEARCH_ALERTS]: { | ||
| REQUEST: (state: Monitors): Monitors => ({ | ||
| ...state | ||
| }), | ||
| SUCCESS: (state: Monitors, action: APIResponseAction): Monitors => ({ | ||
| ...state | ||
| }), | ||
| FAILURE: (state: Monitors, action: APIResponseAction): Monitors => ({ | ||
| ...state | ||
| }), | ||
| }, |
There was a problem hiding this comment.
Maybe don't need to store alert info in redux store, but can you at least update the requesting and errorMessage fields here, similar to others? Can be helpful later on when determining loading states and propagating errors
There was a problem hiding this comment.
Yes, not sure if this will impact other components using requesting and errorMessage. Will add todo here and add it later.
There was a problem hiding this comment.
Sure, not a blocker. If anything, adding requesting and errorMessage wouldn't hurt, as it would just make the frontend more accurate.
There was a problem hiding this comment.
Added toto here, if any client components using these search APIs needs requesting and errorMessage, feel free to resolve this todo..
| [SEARCH_ANOMALY_RESULTS]: { | ||
| REQUEST: (state: Anomalies): Anomalies => ({ | ||
| ...state | ||
| }), | ||
| SUCCESS: (state: Anomalies, action: APIResponseAction): Anomalies => ({ | ||
| ...state | ||
| }), | ||
| FAILURE: (state: Anomalies): Anomalies => ({ | ||
| ...state | ||
| }), | ||
| }, |
| const result = await dispatch( | ||
| searchES( | ||
| getBucketizedAnomalyResultsQuery( | ||
| searchResults(getBucketizedAnomalyResultsQuery( |
There was a problem hiding this comment.
minor: I see this was already like this before, but I feel that overall we should try to avoid making direct calls and using the results directly, since it may cause errors to use those results/responses in downstream functions, etc. This is where redux store can help here in storing data and error messages. Not a blocker obviously
There was a problem hiding this comment.
we have to use direct calls because we are calling same API in multiple places of same component, but current redux store does not support. We have an issue #23 for this.
There was a problem hiding this comment.
I see, thanks for clarifying. Another solution may be making reducer functions more fine-grained and/or storing more fine-grained results in the redux store. Will add suggestion to the issue.
| } catch (err) { | ||
| console.log('Anomaly detector - Unable to search anomaly result', err); | ||
| if (isIndexNotFoundError(err)) { | ||
| return { ok: true, response: { totalDetectors: 0, detectors: [] } }; |
There was a problem hiding this comment.
minor: this response doesn't matter since the reducer isn't using the responses right now to parse anything, but this response doesn't make a lot of sense. Maybe just return empty for now, or at least add TODO to change when we store results in the reducer
There was a problem hiding this comment.
Good catch, will fix this in next PR.
ohltyler
added
the
enhancement
Issues opendistro-for-elasticsearch/anomaly-detection#195
Description of changes:
Change direct index query to API call for FGAC(Fine-Grained Access Control).
Skip flaky test now, will fix it later.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.