)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"d4044f5fa0ea99168887f698721c08a7c79c70f6","unresolved":false,"context_lines":[{"line_number":11,"context_line":""},{"line_number":12,"context_line":"This replaces previous looping which had no guard to prevent unlimited"},{"line_number":13,"context_line":"looping if for some reason DBDuplicateEntry kept happening. This is"},{"line_number":14,"context_line":"possible, rarely, when there are is a small number of threads talking"},{"line_number":15,"context_line":"to the same database, and a small number of aggregates of aggregates"},{"line_number":16,"context_line":"being ensured a huge number of times (thousands) in a very small amount"},{"line_number":17,"context_line":"of time. Under those circumstances a maximum recursion error was"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3f79a3b5_0286a41a","line":14,"range":{"start_line":14,"start_character":29,"end_line":14,"end_character":32},"updated":"2018-12-17 07:32:14.000000000","message":"nix","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"d4044f5fa0ea99168887f698721c08a7c79c70f6","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This replaces previous looping which had no guard to prevent unlimited"},{"line_number":13,"context_line":"looping if for some reason DBDuplicateEntry kept happening. This is"},{"line_number":14,"context_line":"possible, rarely, when there are is a small number of threads talking"},{"line_number":15,"context_line":"to the same database, and a small number of aggregates of aggregates"},{"line_number":16,"context_line":"being ensured a huge number of times (thousands) in a very small amount"},{"line_number":17,"context_line":"of time. Under those circumstances a maximum recursion error was"},{"line_number":18,"context_line":"possible."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3f79a3b5_42981c18","line":15,"range":{"start_line":15,"start_character":55,"end_line":15,"end_character":68},"updated":"2018-12-17 07:32:14.000000000","message":"duplicated","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"}],"placement/objects/resource_provider.py":[{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"c7a18b1f2169f95a7ea97e21646a63016c62fd54","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_0e170644","line":522,"range":{"start_line":522,"start_character":33,"end_line":522,"end_character":34},"updated":"2018-12-11 18:35:16.000000000","message":"up to 5","commit_id":"904ec1d227fbc7978de61cf23612e020e360933a"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"549b8413f021ad26fb25e4a86f9cc9db41ea9ec5","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_8ef796d2","line":522,"range":{"start_line":522,"start_character":33,"end_line":522,"end_character":34},"in_reply_to":"3f79a3b5_0e170644","updated":"2018-12-11 18:41:03.000000000","message":"Done","commit_id":"904ec1d227fbc7978de61cf23612e020e360933a"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"d4044f5fa0ea99168887f698721c08a7c79c70f6","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry up to 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_df9cfa02","line":522,"updated":"2018-12-17 07:32:14.000000000","message":"After retrying the max 5 times, it raises the error, right? Then, the reason why I don\u0027t see the error messages in the perf log [1] is because it is fixed as the retry interval time for the wrap_db_retry working well compared to original just looping method, right?\n\n[1] http://logs.openstack.org/44/624144/2/check/placement-perfload/f787e13/logs/placement-api.log\n\nI think this bug is related deeply with the transaction isolation level [2]... but as long as I can see it is not set in the placement project... I _guess_ it is defaulted to \"READ COMMITTED\", but don\u0027t we want to set and be aware of the level explicitly in the placement project?\n\n[2] https://docs.sqlalchemy.org/en/latest/orm/session_transaction.html#setting-transaction-isolation-levels","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"385454034662f29a494df7a7fefacec876166dcf","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry up to 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_32c0a74b","line":522,"in_reply_to":"3f79a3b5_29acd985","updated":"2018-12-18 09:26:50.000000000","message":"As far as I can find, speaking of the transaction isolation level, MySQL defaults to \"REPEATABLE READ\" [1] and PostgreSQL defaults to \"READ COMMITTED\" [2]. This is not about sqlalchemy, this is about the DB backend itself.\n\n[1] https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-levels.html\n[2] https://www.postgresql.org/docs/9.5/transaction-iso.html\n\n\nWith \"REPEATABLE READ\", within a transaction, one gets exactly the same results from a DB even if another thread commits its  changes to the DB during the transaction. My guess is that that repeatable read is the main cause of the original bug, that is, inserting it fails but reading it says nothing because it reads data from the snapshot of the DB taken when the transaction *started*.\n\n\n...Anyway, if my understanding is right, we\u0027d like to set the the same transaction isolation level for both DB backends and developers should be aware of the level not to cause this kind of bug again.","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"2aaddff4e48ec95d32de57e07ff01e8a6f856f60","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry up to 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_686072d2","line":522,"in_reply_to":"3f79a3b5_32c0a74b","updated":"2018-12-18 10:18:20.000000000","message":"\u003e ...Anyway, if my understanding is right, we\u0027d like to set the the\n \u003e same transaction isolation level for both DB backends and\n \u003e developers should be aware of the level not to cause this kind of\n \u003e bug again.\n\nI\u0027m not sure if that\u0027s the right strategy. I don\u0027t think we can always make assumptions or demands on how people configure their databases. Instead we should make sure that our code works in most common cases.\n\nWhich also means that we want developers to be able to stumble across and fix these kinds of bugs when they get revealed.","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"05039af8921ca65fcd4db03850ae0f2c3973cc84","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry up to 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_9edd0414","line":522,"in_reply_to":"3f79a3b5_686072d2","updated":"2018-12-18 11:56:31.000000000","message":"Okay, then let\u0027s just keep in mind that there are both cases where we get newest data and where we get only stale data within one transaction depending on the kind of the DB back-end.\n\nFor this bug, retrying works well for the former, and fails early here because of the conflict for the latter, both of which look good to me, so this fix can go.","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"458a43212cde29f844a8eaf8595e605157763bdb","unresolved":false,"context_lines":[{"line_number":519,"context_line":"    the aggregate and returns the new aggregate\u0027s internal ID."},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    If there is a race to create the aggregate (which can happen under rare"},{"line_number":522,"context_line":"    high load conditions), retry up to 5 times."},{"line_number":523,"context_line":"    \"\"\""},{"line_number":524,"context_line":"    sel \u003d sa.select([_AGG_TBL.c.id]).where(_AGG_TBL.c.uuid \u003d\u003d agg_uuid)"},{"line_number":525,"context_line":"    res \u003d ctx.session.execute(sel).fetchone()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_29acd985","line":522,"in_reply_to":"3f79a3b5_df9cfa02","updated":"2018-12-17 11:41:13.000000000","message":"The reason you\u0027re not seeing the error in the log is not because of this change. The error is not showing up because perfload itself was changed to spread the requests out over 5 processes with lots of threads. It was before that change that the problem was exposed.\n\nThis change isn\u0027t about making it that problem (with too few threads) go away, but rather to limiting the damage caused by the problem showing up on _other_ processes: when the previous infinite loop was happening it would end up blocking everything.\n\nNow, we retry a few times and then fail the single request. However, because of the use of jitter, the retry ought to be working better.\n\nSo, in general this is just \"better\".\n\nOn your second point about transaction isolation layer, that\u0027s not an area of expertise for me so whatever people think is the right thing. If it is okay and possible to let oslo.db or sqlalchemy do the right thing, for us, then that would be good?","commit_id":"cca30d5c4a0cf5bd702ac5c5b9611e59197f828f"}]}