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

[AMORO-3172] AMS ProcessStatus Miss Running Status #3173

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

czy006
Copy link
Contributor

@czy006 czy006 commented Sep 5, 2024

Why are the changes needed?

Close #3172.

Brief change log

  • Add Running Status in ProcessStatus

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not documented)

Copy link
Contributor

@tcodehuber tcodehuber left a comment

Choose a reason for hiding this comment

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

Is there some conflict between Active and RUNNING status?

@czy006
Copy link
Contributor Author

czy006 commented Sep 5, 2024

Is there some conflict between Active and RUNNING status?

Can use Running Replace Active ? cc @majin1102

@majin1102
Copy link
Contributor

Is there some conflict between Active and RUNNING status?

Can use Running Replace Active ? cc @majin1102

Hi, thanks for pointing this out.

This is an unreasonable reference to ProcessStatus(this belongs to process api which has not been used), the original ref is OptimizingProcess.Status.
I guess this error is introduced when refactoring ams-common module, OptimizingProcess.Status is hard to be pulled out to common thus used ProcessStatus.

Anyway I suggest to add RUNNING and QUEUEING to take place of ACTIVE. QUEUEING means pending external process(submitted but not running)

@czy006 czy006 force-pushed the issues/amoro-3172 branch 2 times, most recently from 7eee079 to dfada0c Compare September 19, 2024 09:19
majin1102

This comment was marked as outdated.

Copy link
Contributor

@majin1102 majin1102 left a comment

Choose a reason for hiding this comment

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

I left one comment, PTAL

Copy link
Contributor

@majin1102 majin1102 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tcodehuber tcodehuber left a comment

Choose a reason for hiding this comment

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

LGTM

@zhoujinsong zhoujinsong merged commit 6ed8162 into apache:master Oct 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: AMS ProcessStatus Miss Running Status
5 participants