-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[index management] Refactor code to allow emitting multiple index lists #249120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,6 @@ import { | |
| pageChanged, | ||
| pageSizeChanged, | ||
| sortChanged, | ||
| loadIndices, | ||
| toggleChanged, | ||
| performExtensionAction, | ||
| } from '../../../../store/actions'; | ||
|
|
@@ -61,9 +60,6 @@ const mapDispatchToProps = (dispatch) => { | |
| toggleChanged: (toggleName, toggleValue) => { | ||
| dispatch(toggleChanged({ toggleName, toggleValue })); | ||
| }, | ||
| loadIndices: () => { | ||
| dispatch(loadIndices()); | ||
| }, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously this was firing off the request and then getting the data back. I need something that could respond to multiple data sets being passed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Totally get the intent, but we don't need RxJS here to achieve this. Redux already provides the “emit + subscribe” mechanism: each Here's an async/await shape (pseudo-code — names like import { v4 as uuidv4 } from 'uuid';
export const loadIndices = () => async (dispatch, getState, { api }) => {
const requestId = uuidv4();
dispatch(loadIndicesStart({ requestId }));
// 1) base list first (renders quickly)
try {
const base = await api.getIndicesBase();
if (getState().indices.currentRequestId !== requestId) return; // ignore stale
dispatch(loadIndicesSuccess({ requestId, indices: base }));
} catch (error) {
if (getState().indices.currentRequestId !== requestId) return; // ignore stale
dispatch(loadIndicesError({ requestId, error }));
return;
}
// 2) slow/optional enrichers in parallel (best-effort, progressive)
const run = async (name, fn) => {
try {
const patch = await fn();
if (getState().indices.currentRequestId !== requestId) return; // ignore stale
dispatch(indicesEnrichmentSuccess({ requestId, name, patch }));
} catch (error) {
if (getState().indices.currentRequestId !== requestId) return; // ignore stale
dispatch(indicesEnrichmentError({ requestId, name, error }));
}
};
void run('stats', () => api.getIndicesStats());
void run('other', () => api.getOtherEnrichment());
// ...repeat for the other calls
};The key idea is just: dispatch base ASAP, then dispatch progressive updates as each slow call returns, and record enrich failures without breaking the base list. |
||
| performExtensionAction: (requestMethod, successMessage, indexNames) => { | ||
| dispatch(performExtensionAction({ requestMethod, successMessage, indexNames })); | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ import { CreateIndexButton } from '../create_index/create_index_button'; | |
| import { IndexTablePagination, PAGE_SIZE_OPTIONS } from './index_table_pagination'; | ||
| import { DocCountCell } from './doc_count'; | ||
| import { docCountApi } from './get_doc_count'; | ||
| import { loadIndices } from '../../../../services/api'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pulling I think it’ll be easier to keep the progressive behavior and keep React/Redux happy if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think I need to learn the redux way of doing things. This is my first time using it in quite a while and this isn't a simple use case so its not immediately clear how to. Sorting my thoughts out in another PR where I'm only concerned with data loading is helpful so I can have a better idea of the specific needs I have for working with the state system. |
||
|
|
||
| const getColumnConfigs = ({ | ||
| showIndexStats, | ||
|
|
@@ -207,7 +208,7 @@ export class IndexTable extends Component { | |
| } | ||
|
|
||
| componentDidMount() { | ||
| this.props.loadIndices(); | ||
| loadIndices(); | ||
|
|
||
| const { filterChanged, pageSizeChanged, pageChanged, toggleNameToVisibleMap, toggleChanged } = | ||
| this.props; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import type { IndicesStatsResponse } from '@elastic/elasticsearch/lib/api/types' | |
| import type { InferenceAPIConfigResponse } from '@kbn/ml-trained-models-utils'; | ||
| import type { MappingTypeMapping } from '@elastic/elasticsearch/lib/api/types'; | ||
| import type { ReindexService } from '@kbn/reindex-service-plugin/public'; | ||
| import { from, Subject, mergeMap } from 'rxjs'; | ||
| import { | ||
| API_BASE_PATH, | ||
| INTERNAL_API_BASE_PATH, | ||
|
|
@@ -142,9 +143,12 @@ export async function updateDSFailureStore( | |
|
|
||
| export async function loadIndices() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this function currently passes a single response to the Subject, the plan is to integrate code that would call a number of endpoints and mush the data together and return it. |
||
| const response = await httpService.httpClient.get<any>(`${API_BASE_PATH}/indices`); | ||
| return response.data ? response.data : response; | ||
| internalLoadIndicesSubject.next(response.data); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this could update the store directly instead of using RxJS
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’d keep |
||
| } | ||
|
|
||
| const internalLoadIndicesSubject = new Subject<{}>(); | ||
| export const loadIndices$ = internalLoadIndicesSubject.asObservable(); | ||
|
|
||
| export async function reloadIndices( | ||
| indexNames: string[], | ||
| { asSystemRequest }: ReloadIndicesOptions = {} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the right place to park this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should pass the
loadIndicesfunction via the AppContextProvider.There's also a
reloadIndicesfunction somewhere in there that I should examine.