better session handling #4
Conversation
lavrukov
force-pushed
the
better-session-handling
branch
8 times, most recently
from
4d73176 to
a25c603
Compare
| @@ -863,6 +866,30 @@ public void schemaWithHint() { | |||
| repository.schema(HintAutoPartitioningByLoad.class).create(); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
todo: please remove excessive newline (same in ydb-repository-v2 probably)
| tx.table(Project.class).save(new Project(id1, "name1")); // make tx available for TLI | ||
|
|
||
| assertThatExceptionOfType(OptimisticLockException.class) | ||
| .isThrownBy(() -> tx.table(Project.class).find(id2)); |
There was a problem hiding this comment.
question: Modern YDB versions can now return OptimisticLock from statements, can they?
There was a problem hiding this comment.
Yes, this test works over latest docker image (23.3).
| assertThatExceptionOfType(OptimisticLockException.class) | ||
| .isThrownBy(() -> tx.table(Project.class).find(id2)); | ||
|
|
||
| tx.commit(); // Commit shouldn't throw an BadSession or other exceptions |
There was a problem hiding this comment.
question: Is it correct to call commit() on a transaction that got invalidated (allowing no-error rollback() after getting OptimisticLock I do understand)?
I think trying to commit an invalidated transaction should definitely cause IllegalStateException or something like that, no?
There was a problem hiding this comment.
In this PR, I wanted to touch the old logic to a minimum. commit() is controlled from TxManger, it cannot make a commit after TLI or BadSession, but maybe someone other can. To reduce the likelihood of strange illegals falling out, I did not check it here. This is what this test shows.
But still, it’s really more correct to throw an exception here, I’ll do it.
Also I have a proposal-PR which make works with session and exceptions more understandable (maybe), but I pull it after this.
lavrukov
force-pushed
the
better-session-handling
branch
2 times, most recently
from
cf4d01c to
82f3a6e
Compare
lavrukov
force-pushed
the
better-session-handling
branch
from
82f3a6e to
31406f1
Compare
|
I will look at the in-memory changes more thoroughly on Monday, January 8. New locking logic seems a bit strange, e.g. you can get a writable tx shard that (seemingly) never checks for TLI |
Correct. I decided not to add lock checking for write operations because I could not reproduce the TLI for this case over a docker container. Looks like write operations can't return TLI right now. I showed it in test writeDontProduceTLI() |
Are we sure that this is the case always, for recent versions of YDB? Maybe it's better to be cautious and raise TLI here even if YDB currently does not. |
We have no any guarantees about future, but now we can't reproduce it, it works without it and legacy in-memory don't have consistency checks on write operations too. Given all this, I don't think we have to enhance checks here. |
This is a good argument :-) I've looked at the code once again. Now I see that throwing TLI from writes will be an optimization, anyway: |
If we received a TLI or BadSession inside a transaction, this means that the transaction was invalidated on the YDB side and sending commit() or rollback() is incorrect, since in this case they always generate a BadSession.
In order to write tests for this behavior that will work both on the real YDB and on the mock, we have to add a check for read locks inside the InMemoryRepository.