Skip to content
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

feat: add UI trigger for query progress #30

Closed
wants to merge 14 commits into from

Conversation

Swiddis
Copy link
Contributor

@Swiddis Swiddis commented Jul 1, 2024

WIP -- for now I've added handling for non-terminal progress, I'm still working on how to use the trigger interface here: https://opensearch-project.github.io/OpenSearch-Dashboards/docs/#/../src/plugins/ui_actions/README. Never used something that requires an ID in a generic like this and getting syntax errors when I try to copy what other plugins are doing, so still WIP.

Issue: #25

@Swiddis Swiddis changed the title feat: add loading progress feat: add UI trigger for query progress Jul 3, 2024
@Swiddis Swiddis marked this pull request as ready for review July 3, 2024 23:22
common/types.ts Outdated Show resolved Hide resolved
common/types.ts Outdated Show resolved Hide resolved
const status = SparkJobState[statusStr as keyof typeof SparkJobState];
switch (status) {
case SparkJobState.SUCCESS:
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR but thought I get your feedback on this function.

when i refactored this a little I kept intact like the returning of a boolean but it returns false because the rxjs operator takeWhile needs to get the false value to stop polling.

but it's confusing to read do you think it's worth to rename this function? or shift the logic so that in the utils function it negates the value here? Because return false when SUCCESS from onPollingSuccess could get confusing.

Copy link
Contributor Author

@Swiddis Swiddis Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be confusing at first, but I think it's generally derivable that the usage is to return false to halt polling and true to continue. There isn't anything inherently "keep polling"-like about true or false.

If we want to make it more clear I think the way to go would be to refactor the polling util to rely on a polling state enum, and return values like Polling.DONE, Polling.CONTINUE, Polling.ERROR. Could even make it more sophisticated with the specific statuses, having things like TIMEOUT or ABORT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's obvious by me introducing JobState to begin with, but I'm a big fan of using state machines and strong typing (i.e. lots of enums) to keep track of program state and handle errors.

@Swiddis Swiddis force-pushed the feature/loading-progress branch from cacf525 to d483695 Compare July 8, 2024 23:23
@Swiddis Swiddis marked this pull request as draft July 10, 2024 18:39
@Swiddis
Copy link
Contributor Author

Swiddis commented Jul 10, 2024

Keeping as draft while I figure out how the caller should consume the actions, might not need to filter by query ID param and might need to include additional info in the action context.

@Swiddis Swiddis marked this pull request as ready for review July 10, 2024 23:30
@Swiddis
Copy link
Contributor Author

Swiddis commented Jul 10, 2024

Alright: got something actually working. Sends query data to the ASYNC_TRIGGER_ID trigger. From the caller side you need to make a Subject that accepts the async query type. (To avoid cross-plugin imports this type also has to be redefined). Here's a sample method to create one of those (given a constant with the same async trigger id, and assuming the trigger ids have been registered):

import { Subject } from 'rxjs';
import { UiActionsStart, createAction } from '../../../ui_actions/public';
import { ASYNC_TRIGGER_ID, ASYNC_ACTION_ID } from '../../common';

interface AsyncQueryContext {
  queryId: string;
  queryStatus: string;
}

export function createQueryProgressSubject(uiActions: UiActionsStart): Subject<AsyncQueryContext> {
  const subject: Subject<AsyncQueryContext> = new Subject();
  uiActions.addTriggerAction(
    ASYNC_TRIGGER_ID,
    createAction<typeof ASYNC_ACTION_ID>({
      id: ASYNC_ACTION_ID,
      execute: async (context: unknown) => subject.next(context as AsyncQueryContext),
    })
  );
  return subject;
}

Then from the plugin start method you can create a subject:

const progressSubject = createQueryProgressAction(uiActions);

And pass this to the front-end where you can subscribe in order to update the progress:

const [progress, setProgress] = useState('NONE');
data.progressSubject.subscribe((v) => setProgress(v.queryStatus));

Pretty roundabout and far from ideal (requires lots of boilerplate to onboard the progress subject + query types + registration + passing all the refs around), but it gets the job done. We'll also require that the progress type is kept backwards-compatible, but otherwise should be pretty manageable.

@ashwin-pc
Copy link

Pretty roundabout and far from ideal (requires lots of boilerplate to onboard the progress subject + query types + registration + passing all the refs around)

Yeah for some of the simpler usecases we should add some abstractions inside the UI Actions plugin itself to make using it easier. Abstracting some this into the UI actions plugin directly would save future developers time here.

We'll also require that the progress type is kept backwards-compatible

What do you mean by this?

@Swiddis
Copy link
Contributor Author

Swiddis commented Jul 11, 2024

If we change the progress object type, other plugins will be depending on it, so it'll be a wider change -- by my count at least 3 plugins are going to be using this interface after it's ready (data/query workbench/observability)

This requirement probably exists already, but since we redefine the type in other plugins it won't be type-checked automatically if we do need to change it and track down all the downstream changes.

The Flint API sometimes returns numeric HTTP statuses as well, for reasons not entirely understood to the commit author. Making the method more robust against any type simplifies using the interface, and checking for null results isn't any more annoying than it was before.

Signed-off-by: Simeon Widdis <[email protected]>
@Swiddis Swiddis force-pushed the feature/loading-progress branch from 74c518f to b70654c Compare July 11, 2024 18:06
Signed-off-by: Simeon Widdis <[email protected]>
@Swiddis
Copy link
Contributor Author

Swiddis commented Jul 22, 2024

@Swiddis Swiddis closed this Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants