Arvados

21703-collection-update-lock @ 6f413a6f0dbf48fb386348ffb7bab44044c26114 -- developer-run-tests: #4302

• All agreed upon points are implemented / addressed.
  • ✅
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • ✅
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • ✅ automated test just does some concurrent updates to check for an easily attained deadlock. As the test comment says, until #21701 it's not possible to check from the outside whether this mechanism is really serializing updates. I figure it makes more sense to test that in #21701 than to go to the trouble of inspecting Rails logs, spying on outgoing calls, etc.
• Documentation has been updated.
  • N/A
• Behaves appropriately at the intended scale (describe intended scale).
  • Expect the additional database queries to be much faster than doing the actual collection update via rails.
• Considered backwards and forwards compatibility issues between client and server.
  • No issues anticipated.
• Follows our coding standards and GUI style guidelines.
  • ✅

My only comment is it would be nice to have trash_sweep tests for both the case where it does delete a row as well as the case where it doesn't (because the row is already locked).

I guess my other only comment is I wish our style guide said something about how to write SQL, particularly how to handle identifiers. I feel like it would be nice if we had some reliable distinction between syntax and identifiers. I don't have a strong preference about whether we might achieve that through capitalization or quoting. But maybe this ship has already sailed too far.

Thanks.

Yes. The rails integration test environment is all about preventing commit from happening at all, sharing database connections, etc., but I eventually realized if the test just uses the postgresql driver directly, it can test both deleting and not-deleting.

Also merged main.

21703-collection-update-lock @ e9e39b5f92ec9a702f2bbdd815dd3a1942820db9 -- developer-run-tests: #4309

Tom Clegg wrote in #note-12:

21703-collection-update-lock @ e9e39b5f92ec9a702f2bbdd815dd3a1942820db9 -- developer-run-tests: #4309

My one nit is it would be good to log the exception that causes ready << false, since it's basically guaranteed to cause a test failure. Otherwise this looks good to me. It tests all the cases and is consistent with our existing tests. Thanks.

Admittedly I was imagining a separate test case. This case is kind of illustrative. If you fail the test, then the actual result you get in the last assertion tells you whether the code is under-deleting or over-deleting. A separate test case doesn't provide any additional information.

But, I didn't catch that detail at first, and I suspect it would've taken me a lot longer to figure out if I didn't already have this code in the L1 cache of my brain. If I was working on something "unrelated" in trash_sweep, and this test starts failing unexpectedly, it'll take me time to read the code, understand what it's testing, and why it's failing.

Separate test cases with good names can help explain the problem to the developer more effectively. If I have a passing test of "trash_sweep deletes inactive locks" and a failing test of "trash_sweep retains active locks," that tells me something about the nature of the bug without having to read the nitty-gritty implementation details of establishing a PostgreSQL connection, what the ready queue is, etc.

As a team we often talk about the tests like they're this black box where you must read the code to understand what they expect. I understand why they feel that way now. But I don't think they have to be that way.

Fixed unlogged exception. Split assert into separate asserts with comments.