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

search()->update() can wipe the entire table #183

Merged
merged 1 commit into from Feb 19, 2015

Conversation

coolo
Copy link
Contributor

@coolo coolo commented Feb 17, 2015

So we better require a fixed DBIx::Class (unfortunately 13.2 is affected by this)

Also don't put 2 now() in one update call to avoid the harmless but still ugly
warning about possibly corrupt data (I don't see any value in setting the started == finished)

also fix one -in call

So we better require a fixed DBIx::Class (unfortunately 13.2 is affected by this)

Also don't put 2 now() in one update call to avoid the harmless but still ugly
warning about possibly corrupt data (I don't see any value in setting the started == finished)

also fix one -in call
@coolo coolo force-pushed the coolo_trying_to_fix_scheduler branch from ecb9519 to 3084878 Compare February 17, 2015 15:07
@@ -689,7 +693,6 @@ sub _job_skip_children{
{
state => OpenQA::Schema::Result::Jobs::DONE,
result => OpenQA::Schema::Result::Jobs::INCOMPLETE,
t_started => now(),
t_finished => now(),
Copy link
Member

Choose a reason for hiding this comment

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

AFAIR scheduled jobs are meant to be set to cancelled rather than done+incomplete. the latter is used for jobs that were running already. And in case of cancelled neither t_started nor t_finished make sense to be set.

@coolo
Copy link
Contributor Author

coolo commented Feb 18, 2015

it's actually very confusing in the webui to see jobs as incomplete because their parent failed

@coolo
Copy link
Contributor Author

coolo commented Feb 18, 2015

btw: I already fixed the package to require the fixed DBIx::Class, so this PR is mainly for cleanup

@nadvornik
Copy link
Contributor

Problem with CANCELLED is that it can be changed back to SCHEDULED. For a job with dependencies it could be problematic. Currently, the job is DONE, job_restart duplicates it and the CHAINED dependency to the parent is dropped during duplication.

So we can either add a new result to indicate that the job failed because of dependencies (maybe use existing SKIPPED?) or handle restarting of CANCELLED job the same as DONE job.

@coolo
Copy link
Contributor Author

coolo commented Feb 18, 2015

SKIPPED sounds good to me

@lnussel
Copy link
Member

lnussel commented Feb 18, 2015

Vladimir Nadvornik schrieb:

Problem with CANCELLED is that it can be changed back to SCHEDULED.
For a job with dependencies it could be problematic. Currently, the
job is DONE, job_restart duplicates it and the CHAINED dependency to
the parent is dropped during duplication.

So the code that restarts resp uncancels jobs needs to be made aware
of chains I guess :-)

So we can either add a new result to indicate that the job failed
because of dependencies (maybe use existing SKIPPED?) or handle
restarting of CANCELLED job the same as DONE job.

We had a similar discussion for jobs that were cancelled because a
new iso was uploaded. A new state obsoleted was introduced for
them. Maybe a new state would be the solution here too indeed.

@coolo
Copy link
Contributor Author

coolo commented Feb 18, 2015

SKIPPED is a result, not a state.

lnussel added a commit that referenced this pull request Feb 19, 2015
search()->update() can wipe the entire table
@lnussel lnussel merged commit 2e4af63 into master Feb 19, 2015
@nadvornik
Copy link
Contributor

I have put my proposal here:
https://progress.opensuse.org/issues/6148

@coolo coolo deleted the coolo_trying_to_fix_scheduler branch February 20, 2015 10:52
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.

None yet

3 participants