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(engine, engine-rest-core) Add a query criteria to retrieve all executing jobs #4494

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

prajwolbhandari1
Copy link
Contributor

related to: #4470

@prajwolbhandari1
Copy link
Contributor Author

@venetrius any update on this?

Copy link
Member

@venetrius venetrius left a comment

Choose a reason for hiding this comment

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

Hi @prajwolbhandari1,

The proposed logic does not exactly query jobs that are currently executing.

select * from act_ru_job where LOCK_EXP_TIME_ is not null and suspension_state_=1 and LOCK_EXP_TIME_ > currenttime

It is possible that LOCK_EXP_TIME_ > currenttime but a job is not executing:
LOCK_EXP_TIME_ is first set when the job executor has acquired the job. However an acquired job might not get executed right away.
If a job failed LOCK_EXP_TIME_ might be updated with an icrement as part of the retry policy

It is also not guaranteed that no job is running if LOCK_EXP_TIME_ <= currenttime. In a case where a job does not return within the LOCK_EXP_TIME_ the job can be acquired by a (different of same) job executor.
I recommend visiting our documentation on the The Job Executor

If the behaviour you want to achieve is still met with this query then one option would be to split executing into two filters.
A query criteria to test if suspension is 1 already exists it is called active
A query criteria that checks if a job has a LOCK_EXP_TIME_ value in the future could be called aquired.

@prajwolbhandari1
Copy link
Contributor Author

Hi @prajwolbhandari1,

The proposed logic does not exactly query jobs that are currently executing.

select * from act_ru_job where LOCK_EXP_TIME_ is not null and suspension_state_=1 and LOCK_EXP_TIME_ > currenttime

It is possible that LOCK_EXP_TIME_ > currenttime but a job is not executing: LOCK_EXP_TIME_ is first set when the job executor has acquired the job. However an acquired job might not get executed right away. If a job failed LOCK_EXP_TIME_ might be updated with an icrement as part of the retry policy

Once the acquisition thread acquires a job, it sets LOCK_EXP_TIME_ in some future time and put that job in the job queue right? Isn't it mean its currently "executing"? However, I agree there might be other possible outcomes after that but at that point when the LOCK_EXP_TIME_ is in future time and suspension_state_ is active, my understanding is executing. Also, I am considering retries as "executing" since its the part of a job that is being executed.

It is also not guaranteed that no job is running if LOCK_EXP_TIME_ <= currenttime. In a case where a job does not return within the LOCK_EXP_TIME_ the job can be acquired by a (different of same) job executor. I recommend visiting our documentation on the The Job Executor

Can the job be acquired though because the suspension_state_ will in active state not in suspended even though LOCK_EXP_TIME_ <= currenttime? The state has to be suspended for it to be picked by another acquisition thread right?

If the behaviour you want to achieve is still met with this query then one option would be to split executing into two filters. A query criteria to test if suspension is 1 already exists it is called active A query criteria that checks if a job has a LOCK_EXP_TIME_ value in the future could be called aquired.

@venetrius Thank you for your reply. Please review my above comments.

Can you please suggest to add/remove from the query that I came up that would retrieve jobs that are currently executing?

@prajwolbhandari1
Copy link
Contributor Author

@venetrius any thoughts?

@prajwolbhandari1
Copy link
Contributor Author

@yanavasileva @venetrius any thoughts on this?

@venetrius
Copy link
Member

Hi @prajwolbhandari1,
I did not have the time to dive in and suggest a change to your query. As soon as I have, I will update you here.

@prajwolbhandari1
Copy link
Contributor Author

Hi @prajwolbhandari1, I did not have the time to dive in and suggest a change to your query. As soon as I have, I will update you here.

@venetrius any thoughts or direction would be helpful.

@venetrius
Copy link
Member

Hi @prajwolbhandari1,
I think the solution you proposed has the downside that users have a different expectation of the result set then what is actually guaranteed.

The API already supports the active flag that is half of the executing check.
Do you think it would make sense for your use case to create a new flag acquired that would check that:

        RES.LOCK_EXP_TIME_ IS NOT NULL
        AND
        RES.LOCK_EXP_TIME_  > #{now, jdbcType=TIMESTAMP}

Then when you can call the API with

{
active: true,
acquired: true
}

the same SQL would be executed then what the current changes would introduce with the executing flag.

@venetrius venetrius added the group:stale DRI: Yana label Dec 12, 2024
@prajwolbhandari1
Copy link
Contributor Author

Hi @prajwolbhandari1, I think the solution you proposed has the downside that users have a different expectation of the result set then what is actually guaranteed.

The API already supports the active flag that is half of the executing check. Do you think it would make sense for your use case to create a new flag acquired that would check that:

        RES.LOCK_EXP_TIME_ IS NOT NULL
        AND
        RES.LOCK_EXP_TIME_  > #{now, jdbcType=TIMESTAMP}

Then when you can call the API with

{
active: true,
acquired: true
}

the same SQL would be executed then what the current changes would introduce with the executing flag.

Perfect @venetrius and it makes sense, let me change the flag from 'executing' to 'acquired'.

@venetrius venetrius added group:requested-info DRI: Yana and removed group:stale DRI: Yana labels Dec 19, 2024
@github-actions github-actions bot added the group:stale DRI: Yana label Dec 20, 2024
Copy link

Closing due to lack of requested feedback. If you would like us to look at this, please provide the requested information to re-open the PR.

@prajwolbhandari1
Copy link
Contributor Author

@venetrius @yanavasileva can you please reopen this PR? I am in process of working on changes. Is it easier to open a new PR together OR reopen this? I am ok with either one.

Thanks!

@venetrius
Copy link
Member

Hi @prajwolbhandari1,
I have reopened this PR.

@venetrius venetrius removed the group:requested-info DRI: Yana label Dec 30, 2024
@prajwolbhandari1
Copy link
Contributor Author

@venetrius Just pushed a new commit. Please take a look at it.

Thanks again @venetrius

@venetrius
Copy link
Member

@venetrius Just pushed a new commit. Please take a look at it.

Thanks again @venetrius

Thanks @prajwolbhandari1,
I will be able to take a look at the end of next week.

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.

2 participants