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

MINOR: DI APP Support Partial Success #18017

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

ulixius9
Copy link
Member

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Copy link
Contributor

The Java checkstyle failed.

Please run mvn spotless:apply in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

Copy link
Contributor

The Java checkstyle failed.

Please run mvn spotless:apply in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@@ -238,7 +238,7 @@ private WorkflowStats processWebAnalytics() {
try {
workflow.process();
} catch (SearchIndexException ex) {
jobData.setStatus(EventPublisherJob.Status.FAILED);
jobData.setStatus(EventPublisherJob.Status.RUNNING);
Copy link
Contributor

Choose a reason for hiding this comment

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

does the job still continue after this ? @ulixius9

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

@sushi30 sushi30 left a comment

Choose a reason for hiding this comment

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

@ulixius9 Can you explain why we need the "Partial success" status? How does this defer from the user's perspective compared to the "failed" or "success" state?

@ulixius9
Copy link
Member Author

@sushi30 the primary goal of this status is to take care of Entity type table a0749875-d042-415b-accb-894ab6bc2977 does not have expected relationship contains to/from entity type databaseSchema type of error. Whenever application fails due to this error a user might think that there is something wrong with the application itself but that's not true right Application works fine but there is some issue with the asset itself. and also the main goal of this pr is also to make such errors ignore-able for AUTs, currently AUTs fails because our application fails with failed status, it will pass if there is a partial success or success status.

We have followed the same logic that we have for ingestion, i.e if more then 90% of the assets succeeded then the application will be marked as partial success.

Copy link

sonarcloud bot commented Sep 30, 2024

@sushi30
Copy link
Contributor

sushi30 commented Sep 30, 2024

I do not see how this PR addresses this issue, unlike the ingestion, this app interacts only with internal components (and not external systems) so we should have a clear state for the app. If this requires the user's attention then we can log a warning, if it does not we can log at a lower level. I suggest making it clear to the user what is the action they should take when the app finishes (retry, reindex, re-ingest etc.). For this case it should be either success (the user can expect DI dashboards to be populated) or failure (look at the logs, resolve the issue and re-run).

For the AUTs I would suggest that we make the failure ignore-able at the test level. I do not think we should make schema changes (which are user-facing interfaces) to address issues with our testing frameworks.

Can we make the app not raise an error for specific types of API errors like the one you stated?

@ulixius9
Copy link
Member Author

I mean the search reindexing application marks the status directly as success in case of such error which IMO might be not apt but we can replicate the same here.

@ulixius9 ulixius9 marked this pull request as draft November 8, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ingestion safe to test Add this label to run secure Github workflows on PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants