PR#16: [frontend] fix trace when forking - copr/copr

copr / copr

#16 [frontend] fix trace when forking

Merged 7 years ago by praiskup. Opened 7 years ago by praiskup.

Unknown source praiskup-fork-traceback-fix into master

[frontend] fix traceback when forking

Pavel Raiskup • 7 years ago

6b3409c

frontend/coprs_frontend/coprs/logic/complex_logic.py

file modified

+2 -1

		`@@ -62,6 +62,7 @@`
		`fbuild = forking.fork_build(build, fcopr, fpackage)`
		`builds_map[fbuild.id] = build.result_dir_name`

		`+ db.session.commit()`
		`ActionsLogic.send_fork_copr(copr, fcopr, builds_map)`
		`return fcopr, created`

		`@@ -234,7 +235,7 @@`
		`for chroot in fbuild.build_chroots:`
		`chroot.status = StatusEnum("forked")`
		`db.session.add(fbuild)`
		`- db.session.commit()`
		`+ db.session.flush()`
		`return fbuild`

		`def create_object(self, clazz, from_object, exclude=list()):`

praiskup commented 7 years ago

I'm not sure why, but I got this traceback error:
InvalidRequestError: This session is in 'committed' state; no
further SQL can be emitted within this transaction.

But anyway, it looks like the fork job should be committed once it
is completely done.

rebased

7 years ago

clime commented 7 years ago

Please, see this and make sure you are not introducing the bug back: https://github.com/fedora-copr/copr/commit/cb2011cb5cb758842d0a886d7bd4aa1120d8b520. Also, it would be good to find out why you get that error.

praiskup commented 7 years ago

There's a loop of COMMITs ATM, which is wrong both because of performance and because it might lead to inconsistent fork action... So it is IMO worth merging anyways.
As I understand it right, regression would be no commit at all, right? And this shouldn't happen?

I am lost in the InvalidRequestError .. maybe we could talk about it in the office and I can present you the issue and try to debug it in person. Based on error spelling it looks like we try to re-commit() transaction which has already been committed ... but I don't see such possibility in code..

Based on google searches, it looks like intermediate commit() causes that some lazy select queries are geting inconsistent ... for example selecting for all 'successful' builds?

I also tested the forking only on my own coprs, perhaps that is special case? Dunno. Maybe this is really a matter of broken autoflush... (for some nasty reason) and flushing explicitly would help.

Edited 7 years ago by praiskup

clime commented on line 13 of frontend/coprs_frontend/coprs/logic/complex_logic.py 7 years ago

There needs to be at least db.session.flush(). Other fbuild.id in the parent method can end up being undefined.

praiskup commented 7 years ago

Ok, thanks for having a look .. I'll experiment a bit and update this pull request with flush().

rebased

7 years ago

praiskup commented 7 years ago

Updated, having flush() instead commit() works. I still don't see why the autoflush feature doesn't work for you (and works for me). Any ideas? PTAL.

praiskup commented 7 years ago

I run copr on f25:
python2-flask-0.11.1-3.fc25.noarch python2-flask-sqlalchemy-2.1-3.fc25.noarch python3-flask-0.11.1-3.fc25.noarch python3-flask-sqlalchemy-2.1-3.fc25.noarch python3-flask-wtf-0.10.0-6.fc25.noarch python-flask-wtf-0.10.0-6.fc25.noarch
While I suppose upstream copr runs f24:
python2-flask-sqlalchemy-2.1-2.fc24.noarch python3-flask-0.10.1-8.fc24.noarch python3-flask-sqlalchemy-2.1-2.fc24.noarch python3-flask-wtf-0.10.0-5.fc24.noarch python-flask-0.10.1-8.fc24.noarch python-flask-wtf-0.10.0-5.fc24.noarch
But I *-sqlalchemy seems to be of the same version.

rebased

7 years ago

clime commented 7 years ago

Looks good. I don't think db.session.add by itself causes flushing. That's why if you need a flush (e.g. to access auto-incremented id of the object added) you should put it there explicitly.

clime commented 7 years ago

Looks good. I don't think db.session.add by itself causes flushing. That's why if you need a flush (e.g. to access auto-incremented id of the object added) you should put it there explicitly.

praiskup commented 7 years ago

I do understand the docs differently, but I haven't found a very deep description of autoflush feature (which is enabled by default).

Thanks for the review && merging.

Pull-Request has been merged by praiskup

7 years ago